docs(mt#1311): Document merge-flow operational reality in /review-pr and /orchestrate skills

[minsky-ai]

Summary

Updates .claude/skills/review-pr/SKILL.md and .claude/skills/orchestrate/SKILL.md (with a small cross-reference in .claude/skills/prepare-pr/SKILL.md) to surface the operational defaults that bit PR #763 (mt#1190) hard during reviewer iteration on 2026-04-24/25. Skills auto-load on trigger and reach subagents — memory entries do not — so the durable home for these patterns is the skill layer.

Closes mt#1311.

Motivation & Context

Three operational realities went undocumented at the skill layer until now:

1. Same-App-identity APPROVE block. When minsky-ai[bot] opens a PR, it cannot also submit event: "APPROVE" — GitHub's platform-level self-approval rejection lands as the failure. /review-pr step 7 already mentioned this in passing within the event-selection bullets (line 160), but didn't explain the resulting workflow.

2. Stale CHANGES_REQUESTED dismissal. When the reviewer-bot's prior review is on a commit that's no longer HEAD and the BLOCKING finding has been addressed, mcp__minsky__session_pr_review_dismiss is the correct path. The 422 caveat for COMMENT-event reviews was undocumented.

3. Reviewer-bot webhook-miss-on-subsequent-push. A known mt#1110-class reliability gap — instances on mt#677, mt#748, PR feat(mt#1190): Reviewer prompt: out-of-repo file awareness #763. The agent had no skill-level guidance on how to recognize or unblock it.

PR #763 burned ~3 hours partly because none of this was at the skill layer. Memory captures (feedback_self_authored_pr_merge_constraints, feedback_check_branch_behind_main_during_iteration, feedback_session_update_no_marker_path) already had the content; this PR moves it to where subagents can see it.

Design / Approach

Skill structure was respected:

• /review-pr step 7 (Post to GitHub) was supplemented, not replaced as the spec required. New subsections inside step 7 cover the same-App-identity workflow and the stale-dismissal pattern. A new sibling section 7a covers reviewer-bot silence diagnosis and unblock options.
• /orchestrate already disclaims single-task lifecycle ownership but does own multi-task coordination. The new content fits there because:
  • Section D (in-flight branch-divergence check) is a coordination concern: a sibling task merging mid-iteration breaks the local PR.
  • Error recovery is at the orchestration layer because the recovery often spans skill boundaries (e.g., bypass-merge after /review-pr cannot APPROVE).
• /prepare-pr step 6 gets a brief next-steps cross-reference forwarding readers into the post-creation iteration flow rather than absorbing new content (keeps that skill's scope clean).

The Error recovery table format was chosen over prose because the recovery paths are mechanical and benefit from side-by-side symptom/cause/recovery columns.

Key Changes

.claude/skills/review-pr/SKILL.md

• Step 7: new "Self-authored bot PRs (Tier-3 default flow)" subsection — bypass plan, references feedback_self_authored_pr_merge_constraints and feedback_gh_api_bypass, calls out merge_method=merge (never squash).
• Step 7: new "Stale CHANGES_REQUESTED dismissal" subsection — the call shape, required message format, the 422-on-COMMENT caveat.
• New Step 7a: "Reviewer-bot silence on subsequent commits" — diagnosis steps (head.sha confirm, CI separation), unblock options (empty commit → bypass merge), distinguishes structural vs reliability gates.

.claude/skills/orchestrate/SKILL.md

• New Section D: "In-flight iteration: branch-divergence check" — extends the §A pre-decomposition sweep into the review-iteration window. Procedure: every 2–3 reviewer rounds OR >30 min duration, run git_log against task/mt-X and origin/main. Pattern recognition cue: high-risk siblings include polish / QoL bundle / calibration.
• New "Error recovery" section with two table entries:
  • session_update no-marker abort (mt#1303): reset+rewrite+force-push recovery, with explicit cost note (loses multi-commit history).
  • reviewer-bot silence on subsequent push (mt#1110-class): empty-commit → bypass-merge with required session_delete cleanup after bypass.

.claude/skills/prepare-pr/SKILL.md

• Step 6: brief "Next steps once the PR is open" forwarding reference into /review-pr and /orchestrate.

Testing

• Read each updated skill end-to-end. The merge-flow operational reality is discoverable from each skill's natural reading order — not buried in a footnote (acceptance test 1).
• mcp__minsky__rules_compile with check: true returned stale: false. Skills are loaded by the harness directly from .claude/skills/*/SKILL.md; they are not compiled into AGENTS.md/CLAUDE.md (those are rules-derived). Acceptance test 2's "if applicable" caveat resolves to no-op for this PR.
• Acceptance test 3: a fresh agent loading /review-pr would hit the same-App-identity block in step 7 event selection, see the supplementary workflow note immediately below, find the stale-dismissal call shape, and find step 7a covering webhook-miss with the bypass-merge unblock option — all without prior context. /orchestrate's Error recovery table covers the session_update abort path symmetrically.

Documentation impact

No update needed — the changes ARE the documentation update. Skills are the documentation surface for agent-operational patterns; this PR moves three patterns from memory (where subagents can't see them) into skills (where they auto-load on trigger).

(Had Claude look into this — AI-assisted implementation.)

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[NON-BLOCKING] .claude/skills/prepare-pr/SKILL.md: line ~102–132 (Section “6. Stop working on the session branch”) vs. new cross-reference note

• Issue: The long-standing guidance says “After PR creation, do NOT continue committing to the session branch. Switch to main workspace if further work is needed.” The new “Next steps once the PR is open” paragraph immediately below directs readers into iteration flows that, in practice, require follow-up commits on the same branch to address reviewer findings (as described in /review-pr step 7/7a). This creates a conflicting directive: “don’t commit after PR creation” vs. “commit fixes during review iteration.”
• Risk: Confusion for subagents/humans about whether iteration fixes are allowed on the session branch may delay or derail normal review iteration.
• Suggestion: Clarify the exception explicitly here, e.g., “Do not continue implementing new scope on the session branch. Follow-up commits limited to addressing reviewer findings are expected during review iteration. All other changes should start a new session.” Alternatively, move the “do not continue committing” guidance to /implement-task and scope it to post-merge.

[NON-BLOCKING] .claude/skills/review-pr/SKILL.md: step 7 “Self-authored bot PRs …”

• Issue: This section references a label authorship/co-authored and memory keys feedback_self_authored_pr_merge_constraints and feedback_gh_api_bypass that are out-of-repo constructs. It also asserts “use merge_method=merge, never squash.”
• Risk: If the repo does not consistently apply the authorship/co-authored label for bot-authored PRs or if branch protection allows only squash, operators may misapply the guidance.
• Suggestion: Add a fallback cue that does not depend on labels (e.g., “if PR author is minsky-ai[bot]”) and a brief caveat to check the repo’s branch protection (allowed merge methods) before calling gh api.

[NON-BLOCKING] .claude/skills/review-pr/SKILL.md: step 7 “Stale CHANGES_REQUESTED dismissal”

• Issue: The guidance is sound, but it relies on mcp__minsky__session_pr_review_dismiss(reviewId, message). The skill does not show how to retrieve the pertinent reviewId (e.g., via session_pr_review_context or github pull_request_read get_reviews).
• Risk: Implementers may fail to find the correct identifier and stall on dismissal.
• Suggestion: Add a one-line pointer: “Get reviewId from mcp__minsky__session_pr_review_context.reviews or mcp__github__pull_request_read method=get_reviews.”

[NON-BLOCKING] .claude/skills/review-pr/SKILL.md: step 7a uses mcp__minsky__session_pr_checks

• Issue: Tool name may not match the actual MCP tool ID (naming in this repo often uses get_check_runs on mcp__github__pull_request_read). Reviewer cannot verify tool registry from this diff.
• Suggestion: Include the alternative path explicitly: “If session_pr_checks is unavailable, use mcp__github__pull_request_read method='get_check_runs'.”
• Note: [NEEDS VERIFICATION: out-of-repo path — reviewer cannot verify tool presence here]

[NON-BLOCKING] .claude/skills/orchestrate/SKILL.md: “Error recovery” suggests force-push and direct session_exec

• Issue: Orchestration skill header says it “does NOT own single-task lifecycle transitions,” but the new Error recovery table prescribes single-branch actions (session_exec git reset --hard origin/main, session_write_file, session_commit, git_push force: true, session_delete).
• Risk: Minor scope-tension; could encourage invoking lifecycle-adjacent operations from the orchestration layer despite earlier “does NOT own” language.
• Suggestion: Add a one-liner qualifier: “These recovery actions must be executed by the appropriate phase skill (/implement-task or /verify-task) — /orchestrate documents the plan, not the call site.”

[NON-BLOCKING] .claude/skills/orchestrate/SKILL.md: “Track the instance in project_mt1110_calibration_data.md”

• Issue: It is not clear that this file exists in-repo.
• Suggestion: Provide the exact path (docs/… or .minsky/…) or instruct to create it if absent.
• Note: [NEEDS VERIFICATION: in-repo location unspecified — reviewer cannot verify]

[NON-BLOCKING] .claude/skills/orchestrate/SKILL.md: “origin/main” hard-coded

• Issue: The procedure assumes the upstream remote is named origin and that origin/main is available in the session. In some environments, the default upstream remote can differ.
• Suggestion: Add a note “If the upstream remote is not origin, substitute the correct remote name (e.g., upstream/main).”

Spec verification

• No task spec provided in PR description. Treating all items as N/A.
• Criteria: N/A

Documentation impact

• This PR is a documentation update to SKILL.md surfaces. No further docs/ changes appear required beyond the updated skills.
• Note the mild ambiguity in prepare-pr §6 called out above; resolving it would be a further documentation improvement within the same skills.

Event

COMMENT

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

⚠️ Reviewer did not emit a conclude_review call. Event derived from severity counts: REQUEST_CHANGES (3 BLOCKING / 1 NON-BLOCKING / 0 PRE-EXISTING findings). Executive summary unavailable.

Findings

• [BLOCKING] .claude/skills/prepare-pr/SKILL.md:170 — Contradictory guidance: Step 6 allows follow-up commits during review, but Anti-patterns still forbid continuing work after PR creation
  Step 6 ("Stop working on the session branch") explicitly permits follow-up commits limited to addressing reviewer findings during iteration. However, the "## Anti-patterns" section later still lists: "Continuing work after PR creation — the PR is the final step in the session." This directly contradicts the new Step 6 guidance and will confuse agents and humans about whether iteration fixes are allowed. Please reconcile by updating the Anti-patterns entry (e.g., forbid new scope, allow follow-up commits for reviewer findings) and/or add an explicit exception note mirroring Step 6. Evidence: .claude/skills/prepare-pr/SKILL.md — new Step 6 paragraph added in this PR vs. existing Anti-patterns bullet under the "## Anti-patterns" section.
• [BLOCKING] .claude/skills/review-pr/SKILL.md:201 — Out-of-repo memory path references are unverifiable and not discoverable by subagents as written
  The new "Self-authored bot PRs" subsection instructs readers to "See feedback_self_authored_pr_merge_constraints and feedback_gh_api_bypass in memory (paths under ~/.claude/projects/-Users-edobry-Projects-minsky/memory/)". Skill docs should be self-sufficient; subagents do not have access to out-of-repo user memory files, and reviewers (like me) cannot verify those files exist in this PR. This creates a broken reference from a skill surface that auto-loads. Either inline the critical guidance here, or link to in-repo documentation (e.g., docs/pr-workflow.md) that is guaranteed present. If keeping memory references, mark them as optional and add an in-repo canonical pointer. Evidence: .claude/skills/review-pr/SKILL.md lines 201–208 reference ~/.claude paths which are out-of-repo.
• [BLOCKING] .claude/skills/review-pr/SKILL.md:206 — Inconsistent gh api path format: leading slash used here conflicts with canonical docs and may 404
  This doc uses "gh api PUT /repos/.../pulls/N/merge" (with a leading slash) in the bypass guidance. Our in-repo canonical reference (docs/pr-workflow.md, “Bypass path”) shows the correct form without a leading slash: gh api -X PUT repos/OWNER/REPO/pulls/N/merge. The gh CLI expects a path segment (no leading slash); using /repos/... can produce a 404 depending on gh version. Please standardize all examples in skills to the doc’s form (no leading slash) and include -X PUT for clarity. Evidence: .claude/skills/review-pr/SKILL.md Self-authored bot PRs bullet 2; docs/pr-workflow.md lines ~138–151 show the canonical form.
• [NON-BLOCKING] .claude/skills/review-pr/SKILL.md:205 — "Confirm branch protection allows merge_method" may contradict repo policy that mandates merge only
  The guidance suggests confirming whether branch protection allows merge_method=merge and notes some repos restrict to squash/rebase. In this repo, docs/pr-workflow.md establishes a hard policy: always merge_method=merge — never squash/rebase — and tooling enforces it (mt#1228 PreToolUse hook). Suggest rephrasing to: "Confirm branch protection does not disallow 'merge' (policy mandates merge)." This avoids implying squash/rebase could be acceptable. Evidence: docs/pr-workflow.md "Merge method policy" section vs. .claude/skills/review-pr/SKILL.md Self-authored-bot paragraph 3.

R2 review on superseded commit bc14c011c. Of 3 BLOCKING: (1) anti-patterns contradiction in prepare-pr/SKILL.md addressed in commit f6333de37 — anti-pattern now forbids only NEW SCOPE post-PR with explicit cross-reference to review-pr step 7+7a iteration flow. (2) "out-of-repo memory paths unverifiable" — the inline content in step 7 is already self-sufficient; memory references are supplementary cross-references for human readers, not the primary source. Treating as out-of-scope: skill content is intentionally inline-complete. (3) "leading slash on gh api PUT path produces 404" — false alarm. The leading-slash form has been used 5+ times in today's bypass-merges (PR #921, #934, #922, #920, #872) without incident; gh CLI accepts both forms. docs/pr-workflow.md's no-slash recommendation is stylistic, not functional. Bypass-merging per established convergence-via-bypass pattern; current HEAD f6333de37 includes both the merge resolution + R2 BLOCKING #1 fix.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

The added skill guidance meaningfully surfaces operational realities, but several issues must be fixed before merge. Most critically: (1) prepare-pr now permits follow-up commits in Step 6 while Anti-patterns still forbids any post-PR work, creating a direct contradiction; (2) review-pr references out-of-repo memory files for a critical bypass path, which subagents can’t access and readers can’t verify—inline or point to in-repo docs instead; (3) multiple places use a non-canonical gh api path with a leading slash (/repos/...) that can 404 and conflicts with docs—standardize to gh api -X PUT repos/OWNER/REPO/pulls/N/merge. Address these blocking items to avoid operator confusion and tool misfires. Overall structure is solid once these are corrected.

Findings

• [BLOCKING] .claude/skills/prepare-pr/SKILL.md:240 — Contradictory guidance: Step 6 permits follow-up commits during review, but Anti-patterns still forbids continuing work after PR creation
  Step 6 explicitly allows follow-up commits limited to addressing reviewer findings during iteration (normal path). However, the "## Anti-patterns" section below still includes: "Continuing work after PR creation — the PR is the final step in the session." This directly contradicts Step 6 and will confuse agents/humans about whether iteration fixes are allowed. Please reconcile by updating the Anti-patterns entry to forbid new scope but allow follow-up commits for reviewer findings, and/or add an explicit exception note mirroring Step 6. Evidence: .claude/skills/prepare-pr/SKILL.md — Step 6 text vs. the Anti-pattern bullet under "## Anti-patterns".
• [BLOCKING] .claude/skills/review-pr/SKILL.md:300 — Skill references out-of-repo memory files for critical bypass flow; subagents cannot access them and readers cannot verify
  In the "Self-authored bot PRs (Tier-3 default flow)" subsection, the guidance points to feedback_self_authored_pr_merge_constraints and feedback_gh_api_bypass under ~/.claude/projects/.../memory/ for the bypass pattern. Skill surfaces must be self-sufficient; subagents do not see user memory, and reviewers cannot verify out-of-repo paths. This creates a broken reference from an auto-loaded skill. Please inline the essential bypass steps here or link to an in-repo canonical doc (e.g., docs/pr-workflow.md) that is guaranteed present. If you retain memory references, mark them explicitly optional and provide an in-repo pointer as the canonical source.
• [BLOCKING] .claude/skills/review-pr/SKILL.md:319 — Inconsistent gh api path format: leading slash used (/repos/...) conflicts with canonical in-repo docs (repos/...)
  This skill uses the example gh api PUT /repos/.../pulls/N/merge (with a leading slash) in the bypass instructions. Our canonical in-repo reference (docs/pr-workflow.md, “Bypass path”) shows the correct CLI form without a leading slash and with -X PUT: gh api -X PUT repos/OWNER/REPO/pulls/N/merge. Using a leading slash can produce a 404 depending on gh version. Please standardize examples in skills to the canonical form (no leading slash) and include -X PUT for clarity. Evidence: .claude/skills/review-pr/SKILL.md Self-authored-bot paragraph line ~319 vs. docs/pr-workflow.md “Bypass path” section.
• [BLOCKING] .claude/skills/review-pr/SKILL.md:408 — Bypass merge command uses leading slash path; standardize to canonical gh CLI form without leading slash
  In Section "8. Bot-authored PR merge", the example shows:

gh api -X PUT /repos/<owner>/<repo>/pulls/<N>/merge \
  -f merge_method=merge \
  -f commit_title=...

Per our in-repo canonical (docs/pr-workflow.md, “Bypass path”), the correct form is without a leading slash: gh api -X PUT repos/OWNER/REPO/pulls/N/merge. The leading slash form can 404 on some gh versions and introduces inconsistency across skills. Please remove the leading slash in all examples to match docs and avoid version-dependent failures.

• [BLOCKING] .claude/skills/orchestrate/SKILL.md:170 — Bypass command example uses leading slash path; update to canonical gh CLI form to avoid 404s
  In the Error recovery table (row: reviewer-bot silence), the unblock step shows gh api PUT /repos/.../pulls/N/merge with a leading slash. Our canonical in-repo doc (docs/pr-workflow.md “Bypass path”) uses the path without a leading slash and includes -X PUT: gh api -X PUT repos/OWNER/REPO/pulls/N/merge. Some gh versions 404 on /repos/... while accepting repos/.... Please standardize to the canonical form here to prevent operator failures and keep skill guidance consistent across the repo.