Repository navigation
fix(merge-pr): count the merge owner's approval as the review - #1067
Conversation
An unattended merge required an approval from the configured required reviewer and stopped when only the operator had approved. That left a pull request stuck even with the strongest review it could get, because no second approval was coming. Now the gate accepts an APPROVED review from the merge owner on the exact current head when the required reviewer hasn't approved, and the merge report names the approving login and head. Required checks, the owner-review label, write authority, and a clean mergeable state still apply. A later push still invalidates the approval. Editing that script trips the changed-file shell check while the shell migration is open, so the shell allow list gains an exact-path exemption list that only the changed-file check honors. Its single entry is this script, and explicit path checks still report every finding for it. Remove the entry when the Go port replaces the script. Assisted-by: codex:gpt-6.1-sol Signed-off-by: Tony Burns <tony@tonyburns.net>
tbhb-agent
added
change:fix
![tbhb-dev-reviews[bot]](https://avatars.githubusercontent.com/u/288164213?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed head: e47d740
Review: CHANGES REQUESTED
Loosens: yes
Owner-tier: yes; five changed merge-agent.sh files match both owner_paths and constitutional_paths. Exact-head owner approval is required; an App review alone cannot authorize this head.
| Live AC | Met on reviewed head | Evidence |
|---|---|---|
| #1053 AC1 | Yes | packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:124-131 accepts the configured owner's exact-head approval as fallback. |
| #1053 AC2 | Yes | packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:128 prints the required login and head. |
| #1053 AC3 | Yes | packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:124-130 uses the same current head for both logins; internal/mergepolicy/approval.go:51 rejects stale or unrelated approvals. |
| #1053 AC4 | Yes | packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:115-116,133-178 retains mergeability, identity, label, policy and required-check gates. |
| #1053 AC5 | No | packages/agents/source/skills/agent-merge-pr/SKILL.codex.md:20 still requires the configured reviewer while :23 accepts the owner instead; docs/agent-skills-migration.md:139 contradicts :141 in the same way. |
#1053 has no comments or Refinement verdict. #864 has a READY verdict (round 3); all twelve Go migration ACs remain for that later slice. This PR does not implement the Go migration and correctly uses a non-closing reference for #864. The present Closes #1053 is premature while AC5 remains unmet. The PR Verification also needs explicit AC1-AC5 IDs with their evidence rather than only a general command table.
Findings:
- P2: Make the operational instructions agree with the fallback. packages/agents/source/skills/agent-merge-pr/SKILL.codex.md:20 and packages/agents/source/skills/agent-merge-pr/SKILL.claude.md:26 still demand an exact-head approval from required_reviewer; their new authority paragraphs permit merge_owner instead. docs/agent-skills-migration.md:139 likewise states the original unconditional requirement. Update those statements to allow either qualifying approval and propagate the generated copies. Otherwise AC5 fails and an agent following step 4 can repeat the refusal this change fixes.
- P2: The requested zero-nolint condition is not met. cmd/shell-allowlist/changed_test.go:4 and cmd/shell-allowlist/run.go:53 retain existing nolint directives. Neither directive was introduced by this patch. No t.Skip or weakened assertion was added.
The required-reviewer fallback is an intentional gate relaxation, and .devtools/shell-allowlist.toml:107 additionally skips the exact source script in changed-file mode. No new loosening-marker suppression was added. The exemption is authorized by #1053's scope; cmd/shell-allowlist/changed_test.go:76-90 checks the neighboring-path and explicit-path boundaries. No additional unneeded guard, option, abstraction or layer was found.
Size: scc 4.1.0 measured 43 changed code lines and 9 nonblank prose lines (additions plus deletions; the gate's vendor, generated-copy, token and lock exclusions applied), within 800/2000. Raw churn is 197 lines. No size P1.
Checks: all 23 observed contexts were COMPLETED SUCCESS. Coverage: SUCCESS. CodeQL: SUCCESS, including Go and Actions analysis. GitHub reports MERGEABLE, BLOCKED; ready, not draft. No exact-head approval was present in the reviews read. Private-list leak scans passed for all 27 changed files, one invocation per file. Mechanical gates were not rerun.
Changed paths from git diff --name-status origin/main...HEAD:
| Status / path | Owner-tier | Constitutional |
|---|---|---|
| M .agents/skills/agent-merge-pr/SKILL.md | no | no |
| M .agents/skills/agent-merge-pr/tokens.json | no | no |
| M .agents/skills/merge-pr/scripts/merge-agent.sh | yes | yes |
| M .agents/skills/merge-pr/tokens.json | no | no |
| M .claude/skills/agent-merge-pr/SKILL.md | no | no |
| M .claude/skills/agent-merge-pr/tokens.json | no | no |
| M .claude/skills/merge-pr/scripts/merge-agent.sh | yes | yes |
| M .claude/skills/merge-pr/tokens.json | no | no |
| M .devtools/shell-allowlist.toml | no | no |
| M AGENTS.md | no | no |
| M apm.lock.yaml | no | no |
| M cmd/shell-allowlist/changed_test.go | no | no |
| M cmd/shell-allowlist/run.go | no | no |
| M docs/agent-skills-migration.md | no | no |
| M docs/shell-allowlist.md | no | no |
| M internal/shell/shell.go | no | no |
| M packages/agents/claude/.apm/skills/agent-merge-pr/SKILL.md | no | no |
| M packages/agents/claude/.apm/skills/agent-merge-pr/tokens.json | no | no |
| M packages/agents/claude/.apm/skills/merge-pr/scripts/merge-agent.sh | yes | yes |
| M packages/agents/claude/.apm/skills/merge-pr/tokens.json | no | no |
| M packages/agents/codex/.apm/skills/agent-merge-pr/SKILL.md | no | no |
| M packages/agents/codex/.apm/skills/agent-merge-pr/tokens.json | no | no |
| M packages/agents/codex/.apm/skills/merge-pr/scripts/merge-agent.sh | yes | yes |
| M packages/agents/codex/.apm/skills/merge-pr/tokens.json | no | no |
| M packages/agents/source/skills/agent-merge-pr/SKILL.claude.md | no | no |
| M packages/agents/source/skills/agent-merge-pr/SKILL.codex.md | no | no |
| M packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh | yes | yes |
Policy patterns at .agents/agent-skills.conf:9 and :5:
owner_paths = ^(\.agents/(agent-skills\.conf|review-instructions\.md)$|\.github/rulesets/|\.claude/settings\.json$|packages/agents/source/(lib/(lib|agent)\.sh$|skills/[^/]+/(shared|claude|codex)/scripts/guard[^/]*\.sh$|skills/merge-pr/(shared|claude|codex)/scripts/(merge-agent\.sh|merge-policy\.py)$)|(packages/agents/(claude|codex)/\.apm/skills|\.(claude|agents)/skills)/([^/]+/scripts/(guard[^/]*\.sh|(lib|agent)\.sh)$|(merge-pr|agent-merge-pr)/scripts/(merge-agent\.sh|merge-policy\.py)$))
constitutional_paths = ^(\.agents/(agent-skills\.conf|review-instructions\.md)$|\.github/(rulesets/|CODEOWNERS$)|\.claude/settings\.json$|packages/agents/source/(lib|skills/merge-pr/(shared|claude|codex)/scripts)/(merge-policy\.py|merge-agent\.sh|signing\.sh|lib\.sh)$|(packages/agents/[^/]+/\.apm/skills|\.(claude|agents)/skills)/[^/]+/scripts/(merge-policy\.py|merge-agent\.sh|signing\.sh|lib\.sh)$)
Owner-path-only matches: none. Every matching changed path is constitutional and requires owner approval of the reviewed head.
Round: 1 of 3
The unattended merge script already accepts an exact-head approval from the merge owner when the required reviewer hasn't approved. The agent-merge-pr step 4 text and the migration note still said the merge needs the required reviewer's approval. An agent following step 4 would stop at the refusal this fallback exists to avoid. Both statements describe the fallback, and the generated skill copies carry it. Assisted-by: claude:claude-sonnet-5-5 Signed-off-by: Tony Burns <tony@tonyburns.net>
There was a problem hiding this comment.
Reviewed head: 176af34
Review: APPROVED
Loosens: yes
Owner-tier: yes. Five changed merge-agent.sh paths are constitutional; exact-head owner approval is still required after this approval. The review App alone does not authorize merging this head.
| Live AC | Met on this exact head | Evidence |
|---|---|---|
| #1053 AC1 | Yes | packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:124 accepts merge_owner approval when required_reviewer has not approved. |
| #1053 AC2 | Yes | packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:128 prints the approving login and head. |
| #1053 AC3 | Yes | packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:127 uses the same current head; :130 retains WAITING_REVIEW. internal/mergepolicy/approval.go:51 rejects stale or unrelated approvals. |
| #1053 AC4 | Yes | packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:115 and :133 retain identity, readiness, label, policy and required-check behavior; the diff changes only reviewer fallback. |
| #1053 AC5 | Yes | packages/agents/source/skills/agent-merge-pr/SKILL.codex.md:20 and packages/agents/source/skills/agent-merge-pr/SKILL.claude.md:26 now allow the fallback, matching their authority paragraphs and generated copies; docs/agent-skills-migration.md:139 and AGENTS.md:72 agree. |
Findings: none. Round 1's AC5 contradiction is resolved. Both existing nolint directives are present on origin/main (cmd/shell-allowlist/changed_test.go:4 and cmd/shell-allowlist/run.go:53); this diff adds zero. No t.Skip or weakened assertion was added. merge-agent.sh is unchanged since round 1.
The PR Verification explicitly covers AC1-AC5 (AC1 and AC2 share one row). Closes #1053 is supported; no #1053 AC is deferred. That issue has no comments or Refinement verdict. #864 has a READY Refinement verdict; its AC1-AC12 remain for the later Go migration, and its non-closing reference is appropriate.
The approval fallback intentionally loosens the gate. .devtools/shell-allowlist.toml:107 also exempts the exact source script only in changed-file mode; cmd/shell-allowlist/run.go:37 enforces that boundary, and cmd/shell-allowlist/changed_test.go:76 checks neighboring and explicit paths. No additional loosening-marker suppression or unnecessary guard, option, abstraction or layer was found. Constitutional and loosened-gate findings remain outside the App-only golden-file waiver at packages/agents/source/skills/merge-pr/shared/scripts/merge-policy.py:576.
Size: scc 4.1.0 measured 43 changed code lines and 15 nonblank prose lines, additions plus deletions, excluding vendor, generated copies, token metadata and the lockfile as in the size gate. Within 800/2000; no size P1. Raw churn: 211 lines.
Checks: all 23 contexts read were COMPLETED SUCCESS. Coverage: SUCCESS. CodeQL: SUCCESS, including Go and Actions analysis. Ready, not draft; MERGEABLE, BLOCKED. Private-list leak scans passed for all 27 changed files, one invocation per file. Mechanical gates were not rerun.
Changed paths against origin/main:
| Status / path | Owner-tier | Constitutional |
|---|---|---|
| M .agents/skills/agent-merge-pr/SKILL.md | no | no |
| M .agents/skills/agent-merge-pr/tokens.json | no | no |
| M .agents/skills/merge-pr/scripts/merge-agent.sh | yes | yes |
| M .agents/skills/merge-pr/tokens.json | no | no |
| M .claude/skills/agent-merge-pr/SKILL.md | no | no |
| M .claude/skills/agent-merge-pr/tokens.json | no | no |
| M .claude/skills/merge-pr/scripts/merge-agent.sh | yes | yes |
| M .claude/skills/merge-pr/tokens.json | no | no |
| M .devtools/shell-allowlist.toml | no | no |
| M AGENTS.md | no | no |
| M apm.lock.yaml | no | no |
| M cmd/shell-allowlist/changed_test.go | no | no |
| M cmd/shell-allowlist/run.go | no | no |
| M docs/agent-skills-migration.md | no | no |
| M docs/shell-allowlist.md | no | no |
| M internal/shell/shell.go | no | no |
| M packages/agents/claude/.apm/skills/agent-merge-pr/SKILL.md | no | no |
| M packages/agents/claude/.apm/skills/agent-merge-pr/tokens.json | no | no |
| M packages/agents/claude/.apm/skills/merge-pr/scripts/merge-agent.sh | yes | yes |
| M packages/agents/claude/.apm/skills/merge-pr/tokens.json | no | no |
| M packages/agents/codex/.apm/skills/agent-merge-pr/SKILL.md | no | no |
| M packages/agents/codex/.apm/skills/agent-merge-pr/tokens.json | no | no |
| M packages/agents/codex/.apm/skills/merge-pr/scripts/merge-agent.sh | yes | yes |
| M packages/agents/codex/.apm/skills/merge-pr/tokens.json | no | no |
| M packages/agents/source/skills/agent-merge-pr/SKILL.claude.md | no | no |
| M packages/agents/source/skills/agent-merge-pr/SKILL.codex.md | no | no |
| M packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh | yes | yes |
Policy at .agents/agent-skills.conf:9 and :5:
owner_paths = ^(\.agents/(agent-skills\.conf|review-instructions\.md)$|\.github/rulesets/|\.claude/settings\.json$|packages/agents/source/(lib/(lib|agent)\.sh$|skills/[^/]+/(shared|claude|codex)/scripts/guard[^/]*\.sh$|skills/merge-pr/(shared|claude|codex)/scripts/(merge-agent\.sh|merge-policy\.py)$)|(packages/agents/(claude|codex)/\.apm/skills|\.(claude|agents)/skills)/([^/]+/scripts/(guard[^/]*\.sh|(lib|agent)\.sh)$|(merge-pr|agent-merge-pr)/scripts/(merge-agent\.sh|merge-policy\.py)$))
constitutional_paths = ^(\.agents/(agent-skills\.conf|review-instructions\.md)$|\.github/(rulesets/|CODEOWNERS$)|\.claude/settings\.json$|packages/agents/source/(lib|skills/merge-pr/(shared|claude|codex)/scripts)/(merge-policy\.py|merge-agent\.sh|signing\.sh|lib\.sh)$|(packages/agents/[^/]+/\.apm/skills|\.(claude|agents)/skills)/[^/]+/scripts/(merge-policy\.py|merge-agent\.sh|signing\.sh|lib\.sh)$)
Owner-path-only matches: none. Every owner-tier match above is constitutional and requires approval of this exact head by the owner.
Round: 2 of 3
Summary
The unattended merge gate accepts an APPROVED review from the merge owner on the exact current head when the required reviewer hasn't approved. The merge report prints the approving login and head. The shell allow list also gains an exact-path exemption list that only its changed-file check honors, and it lists
merge-agent.sh.Why
An unattended merge stopped at
WAITING_REVIEWwhenever only the operator had approved, so a pull request with the strongest review available couldn't merge. The operator's exact-head approval now satisfies the reviewer check. Required checks, the owner-review label, write authority and a clean mergeable state still apply, and a later push still invalidates the approval.Editing the script trips the changed-file shell check while the shell migration is open, so the exemption keeps that check from blocking
merge-agent.sh. Explicit path checks still report every finding for it. Remove the entry when the Go port in #864 replaces the script.Verification
The commands below ran on the head of this pull request. Criteria are the acceptance criteria of #1053.
sed -n 118,134p packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.shreviewer satisfied by merge_owner: <login> approved <head>sed -n 124,131p packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh$head, and any other outcome stops withWAITING_REVIEWgit diff origin/main...HEAD -- packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.shmise run check-generated-skillsmerge_ownermise run test-gomise run lint-shellThe
//nolintdirectives atcmd/shell-allowlist/changed_test.go:4andcmd/shell-allowlist/run.go:53already exist onmain.git diff origin/main...HEAD -- cmd/shell-allowlist | grep nolintmatches nothing in the added lines, so this change adds none.Risk
Compatibility: none
Loosens: yes
This loosens the merge gate: an exact-head approval by
merge_ownernow counts as therequired_reviewerapproval. An unset or wrongmerge_ownerlogin makes the fallback never match, and the merge still stops atWAITING_REVIEW. A person must holdmerge_owner, because an App could otherwise approve its own change. Revert the commit to restore the previous gate and remove the exemption.Related
Closes #1053. Part of #864.