Skip to content

feat: reviewer discipline gate for free-tier private repos (ship + land-and-deploy) #884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Tarabcak wants to merge 1 commit into
base: main
Choose a base branch
from
+94 −4
Open

feat: reviewer discipline gate for free-tier private repos (ship + land-and-deploy) #884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions land-and-deploy/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,46 @@ and tell the user: "I found and fixed a few issues during the review. The fixes

**If review is CURRENT:** Skip this sub-step entirely — no question asked.

### 3.5a-ter: GitHub PR review approval check (HARD GATE)

**This is a hard gate — it blocks merge if a human reviewer was requested but hasn't approved.** Local AI reviews (3.5a) are not a substitute for human approval when a reviewer is assigned.

Query the PR's review state:

```bash
gh pr view --json reviewDecision,reviewRequests,latestReviews
```

Parse the output:
- `reviewDecision`: `APPROVED` / `CHANGES_REQUESTED` / `REVIEW_REQUIRED` / `null`
- `reviewRequests`: list of pending reviewers (haven't responded yet)
- `latestReviews`: list of reviews submitted (each has a `state` and `author`)

**Gate logic:**

| Condition | Action |
|-----------|--------|
| `reviewDecision == "APPROVED"` | PASS — continue |
| `reviewDecision == "CHANGES_REQUESTED"` | **BLOCKER** — list the reviewer who requested changes, the file/line they commented on if available, and refuse to merge |
| `reviewDecision == "REVIEW_REQUIRED"` AND `reviewRequests` is non-empty | **BLOCKER** — "PR has {N} pending reviewer(s): {list}. Cannot merge until at least one approves." |
| `reviewDecision == null` AND `reviewRequests` is empty AND `latestReviews` is empty AND repo has other collaborators | **WARNING** — "No reviewers requested. This repo has other collaborators — consider requesting review before merge." Not a hard blocker because it may be a solo-authored fix in a multi-person repo. |
| `reviewDecision == null` AND repo has no other collaborators | PASS — solo repo, no review possible |

**How to fetch collaborators for the "solo repo" check:**

```bash
_SELF=$(gh api user --jq .login 2>/dev/null)
_OTHER_COLLABS=$(gh api repos/:owner/:repo/collaborators --jq "[.[] | select(.login != \"$_SELF\") | .login] | length" 2>/dev/null || echo "0")
```

If `_OTHER_COLLABS > 0`, treat missing reviews as a warning/blocker per the table above. If `_OTHER_COLLABS == 0`, treat as solo repo and pass.

**If BLOCKER:** Include the reviewDecision status in the readiness report under a new `GITHUB REVIEW` section (see 3.5e) and recommend option B (hold off). Do NOT let the user override with option C for this specific blocker — review bypass requires a different, explicit override.

**Explicit override for the GitHub review blocker:** Only if the user types `/land-and-deploy --override-review` (or answers an explicit `AskUserQuestion` with a dedicated "I am the reviewer and I approve this" option) can this blocker be bypassed. Default is refuse.

**Why this matters:** Free GitHub plans on private repos have no branch protection, so discipline is the only gate. Without this check, `/land-and-deploy` would happily merge PRs that the assigned reviewer never saw. That is the exact failure mode this sub-step exists to prevent.

### 3.5b: Test results

**Free tests — run them now:**
Expand Down Expand Up @@ -1126,12 +1166,18 @@ Build the full readiness report:
║ PR: #NNN — title ║
║ Branch: feature → main ║
║ ║
║ REVIEWS ║
║ REVIEWS (local AI) ║
║ ├─ Eng Review: CURRENT / STALE (N commits) / — ║
║ ├─ CEO Review: CURRENT / — (optional) ║
║ ├─ Design Review: CURRENT / — (optional) ║
║ └─ Codex Review: CURRENT / — (optional) ║
║ ║
║ GITHUB REVIEW (human) ║
║ ├─ Decision: APPROVED / CHANGES_REQUESTED / ║
║ │ REVIEW_REQUIRED / none (solo repo) ║
║ ├─ Approvers: @user1 / — ║
║ └─ Pending: @user2 / — ║
║ ║
║ TESTS ║
║ ├─ Free tests: PASS / FAIL (blocker) ║
║ ├─ E2E tests: 52/52 pass (25 min ago) / NOT RUN ║
Expand All @@ -1149,7 +1195,8 @@ Build the full readiness report:
╚══════════════════════════════════════════════════════════╝
```

If there are BLOCKERS (failing free tests): list them and recommend B.
If there are BLOCKERS (failing free tests, OR `reviewDecision == "CHANGES_REQUESTED"`, OR pending required reviewers from 3.5a-ter): list them and recommend B.
**If the blocker is "GitHub PR review not approved", option C is NOT available.** The user must either get the approval on GitHub or explicitly pass `--override-review` to `/land-and-deploy`. Do not present option C for this blocker.
If there are WARNINGS but no blockers: list each warning and recommend A if
warnings are minor, or B if warnings are significant.
If everything is green: recommend A.
Expand Down Expand Up @@ -1575,6 +1622,7 @@ Then suggest relevant follow-ups:

- **Never force push.** Use `gh pr merge` which is safe.
- **Never skip CI.** If checks are failing, stop and explain why.
- **Never merge a PR without approved human review when a reviewer was assigned.** Step 3.5a-ter is a HARD GATE — if `reviewDecision != "APPROVED"` and the PR has reviewers or the repo has other collaborators, refuse to merge. The only override is an explicit `--override-review` flag or an explicit AskUserQuestion bypass. Do not treat local AI reviews (eng-review, codex-review) as a substitute for human PR approval. On free GitHub plans with no branch protection, this discipline is the only thing preventing unreviewed merges to main.
- **Narrate the journey.** The user should always know: what just happened, what's happening now, and what's about to happen next. No silent gaps between steps.
- **Auto-detect everything.** PR number, merge method, deploy strategy, project type, merge queues, staging environments. Only ask when information genuinely can't be inferred.
- **Poll with backoff.** Don't hammer GitHub API. 30-second intervals for CI/deploy, with reasonable timeouts.
Expand Down
46 changes: 44 additions & 2 deletions ship/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2453,27 +2453,68 @@ you missed it.>
🤖 Generated with [Claude Code](https://claude.com/claude-code)
```

**Reviewer resolution (before creating the PR):**

Check for a configured reviewer in this priority order:
1. `CLAUDE.md` has a line matching `Default reviewer:\s*@?(\w+)` — use that username.
2. `CLAUDE.md` has a line matching `Reviewers?:\s*@?(\w+(?:,\s*@?\w+)*)` — use that list.
3. Otherwise: `gh api repos/:owner/:repo/collaborators --jq "[.[] | select(.login != \"$(gh api user --jq .login)\") | .login] | join(\",\")" 2>/dev/null` — auto-detect non-self collaborators.
4. If none of the above return anything, skip reviewer assignment (solo repo, no reviewers available).

Store the result in a variable, e.g. `_REVIEWERS`.

**If GitHub:**

```bash
gh pr create --base <base> --title "<type>: <summary>" --body "$(cat <<'EOF'
# Create PR with reviewer if resolved
if [ -n "$_REVIEWERS" ]; then
gh pr create --base <base> --reviewer "$_REVIEWERS" --title "<type>: <summary>" --body "$(cat <<'EOF'
<PR body from above>
EOF
)"
else
gh pr create --base <base> --title "<type>: <summary>" --body "$(cat <<'EOF'
<PR body from above>
EOF
)"
fi
```

**If GitLab:**

```bash
glab mr create -b <base> -t "<type>: <summary>" -d "$(cat <<'EOF'
# GitLab equivalent: --reviewer flag
if [ -n "$_REVIEWERS" ]; then
glab mr create -b <base> --reviewer "$_REVIEWERS" -t "<type>: <summary>" -d "$(cat <<'EOF'
<MR body from above>
EOF
)"
else
glab mr create -b <base> -t "<type>: <summary>" -d "$(cat <<'EOF'
<MR body from above>
EOF
)"
fi
```

**If neither CLI is available:**
Print the branch name, remote URL, and instruct the user to create the PR/MR manually via the web UI. Do not stop — the code is pushed and ready.

**Post-creation review gate banner (MANDATORY if `_REVIEWERS` is set):**

After the PR/MR is created, print this banner verbatim and STOP any auto-merge intent:

```
==================================================================
REVIEW REQUESTED from: $_REVIEWERS
DO NOT MERGE this PR until a reviewer approves it.
Check status: gh pr view <PR#> --json reviewDecision,reviews
Merge only when reviewDecision == "APPROVED"
==================================================================
```

**Never run `gh pr merge` (or equivalent) in the same /ship invocation when reviewers are set.** /ship's job ends at PR creation. The user runs a separate merge command only after the review is approved. If the user asks /ship to also merge, refuse with: "Review required first. PR #<N> has reviewers assigned. Merge only after reviewDecision is APPROVED."

**Output the PR/MR URL** — then proceed to Step 8.5.

---
Expand Down Expand Up @@ -2532,6 +2573,7 @@ This step is automatic — never skip it, never ask for confirmation.
- **Never skip tests.** If tests fail, stop.
- **Never skip the pre-landing review.** If checklist.md is unreadable, stop.
- **Never force push.** Use regular `git push` only.
- **Never merge without reviewer approval.** If the repo has collaborators or CLAUDE.md specifies a reviewer, /ship creates the PR with `--reviewer` assigned and STOPS. The user runs `gh pr merge` only after `reviewDecision == "APPROVED"`. If asked to merge in the same invocation, refuse and point at the review gate banner from Step 8.
- **Never ask for trivial confirmations** (e.g., "ready to push?", "create PR?"). DO stop for: version bumps (MINOR/MAJOR), pre-landing review findings (ASK items), and Codex structured review [P1] findings (large diffs only).
- **Always use the 4-digit version format** from the VERSION file.
- **Date format in CHANGELOG:** `YYYY-MM-DD`
Expand Down