T-8896-add-pr-check-skill

Status: closed/done · Impact: high · Complexity: small

Goal

Ship /sdlc:pr-check <pr-number> — a single-PR classifier that the orchestrator calls per open PR each tick. Returns one of a fixed enum ({CLEAN, NEEDS-RESPONSE, CONFLICTS, CI-FAILED, MERGED}) plus a one-line reason. Tight, predictable contract is the point — the orchestrator dispatches on the return verdict and never has to parse free-form PR state itself.

Today

There is no skill for triaging an individual PR. The relevant signals are scattered across gh calls:

Merge state: gh pr view <N> --json mergeable,mergeStateStatus,state — state can be OPEN/CLOSED/MERGED; mergeable is MERGEABLE/CONFLICTING/UNKNOWN; mergeStateStatus adds CLEAN/UNSTABLE/BLOCKED/DIRTY/BEHIND.
New review comments: gh api repos/{owner}/{repo}/pulls/{n}/comments + gh api repos/{owner}/{repo}/issues/{n}/comments — needs a “last orchestrator visit” timestamp to know what’s new.
CI: gh pr checks <N> — exit code + table output.
Review state: gh pr view <N> --json reviews,reviewDecision — reviewDecision is APPROVED/CHANGES_REQUESTED/REVIEW_REQUIRED.

Composing these into a single verdict every tick from inside /sdlc:orchestrate would balloon the orchestrator skill prose and bury the dispatch logic. Pulling it into its own skill keeps /sdlc:orchestrate focused on the loop and lets pr-check evolve its signal-fusion logic independently.

Proposed

Skill at plugin/skills/pr-check/SKILL.md invocable as /sdlc:pr-check <pr-number>. Procedure:

Resolve PR number; bail with ERROR: not found if gh pr view <N> fails.
Fetch state in a single gh pr view <N> --json state,mergeable,mergeStateStatus,reviewDecision,headRefName,baseRefName,statusCheckRollup call.
Classify in priority order (first match wins):
- state == "MERGED" → return MERGED reason="merged at <mergedAt>; task=<derived from headRefName>".
- state == "CLOSED" (unmerged) → return CLOSED reason="closed without merge". (Orchestrator treats this same as MERGED for close-out purposes — task-close-out handles the closed/wontdo path; revisit if it doesn’t.)
- mergeable == "CONFLICTING" OR mergeStateStatus == "DIRTY" → return CONFLICTS reason="<which files>".
- Any check in statusCheckRollup failing → return CI-FAILED reason="<which check>".
- reviewDecision == "CHANGES_REQUESTED" OR new review comments since last visit → return NEEDS-RESPONSE reason="<who, count>".
- Otherwise → return CLEAN reason="awaiting review" or CLEAN reason="approved, awaiting merge".
“New since last visit” uses a per-PR cursor file at .claude/pr-cursors/<pr-number>.json storing the timestamp of the most recent comment last seen. Cursor is updated on every invocation. Cursor files are gitignored.

The skill is read-only on the repo and on GitHub — it never comments, merges, or modifies files outside .claude/pr-cursors/. Action is the orchestrator’s job; classification is this skill’s.

Approach

Scaffold plugin/skills/pr-check/SKILL.md with allowed-tools: [Bash, Read, Write].
Write the classification procedure with the priority-ordered ladder above.
Define the cursor-file format and add .claude/pr-cursors/ to .gitignore (or confirm .claude/ already is).
Add plugin/skills/pr-check/invariants.yaml: “skill must be read-only on the repo and GitHub except for .claude/pr-cursors/”; “return must match the documented enum exactly”; “priority order is fixed”.
Verify against current open PRs (#21–#24 plus any new ones): each should classify cleanly into one of the five verdicts.

Files to touch

plugin/skills/pr-check/SKILL.md (new) — classification procedure.
plugin/skills/pr-check/invariants.yaml (new) — lint invariants.
.gitignore — confirm .claude/pr-cursors/ is ignored.

Acceptance criteria

AC-1: /sdlc:pr-check <N> returns exactly one line matching <VERDICT> reason="..." where VERDICT ∈ {CLEAN, NEEDS-RESPONSE, CONFLICTS, CI-FAILED, MERGED, CLOSED, ERROR}. (auto)
AC-2: Run against each currently-open PR (#21–#24 or whatever exists at land time); each classifies into a verdict that a human reviewer would agree with. (agent-manual)
AC-3: Cursor file at .claude/pr-cursors/<N>.json is created on first call and updated on subsequent calls; new comments arriving between calls flip the verdict to NEEDS-RESPONSE. (agent-manual: post a test comment, re-invoke, confirm)
AC-4: Skill makes no gh pr comment, gh pr merge, gh pr close, or git commit calls. Verifiable by lint invariant. (auto)
AC-5: lint_skill_prose.py passes against the new invariants.yaml. (auto)

Out of scope

Acting on the verdict (rebasing, commenting, merging). That’s the orchestrator’s job.
Multi-PR fan-out. This skill handles one PR at a time; the orchestrator iterates.
GitHub Actions / webhook integration. We poll via gh for Phase 1.
Distinguishing flaky CI from real failures. Any failed check = CI-FAILED.

Dependencies

none

Discovery context

Phase 1 of epic T-FQCN-self-driving-orchestrator-loop. Sibling of T-3OVF-add-orchestrate-skill and T-RDKI-extract-task-close-out-skill.

Post-mortem

Captured by /sdlc:task-work on 2026-05-20. PR: pending.

Acceptance criteria coverage

AC-1: auto — plugin/scripts/lint_skill_prose.py (via the new invariants.yaml) asserts every verdict token (CLEAN, NEEDS-RESPONSE, CONFLICTS, CI-FAILED, MERGED, CLOSED, ERROR) is present under the “Output contract” section. Lint passes 3/3 (pr-check + task-work + import-planning) with no violations.
AC-2: agent-manual — walked the priority ladder against PR #38: gh pr view 38 --json state,... returned state == "MERGED", so rule 1 fires and the emitted verdict is MERGED reason="merged at 2026-05-20T02:15:53Z; task=feat/2026-05-19-add-obsidian-bases-setup-flag" — matches what a human reviewer would say. No currently-open PRs exist to exercise the other verdicts; rules 2-6 are spec-only at this point.
AC-3: deferred-user — cursor file shape (.claude/pr-cursors/<N>.json with last_seen_comment_at
- last_invoked_at) is fully specified in SKILL.md Step 4, but the “new comment flips verdict to NEEDS-RESPONSE” half cannot be exercised in this environment without a live open PR. To finalise, open a draft PR, run pr-check, post a third-party comment, run pr-check again — confirm verdict flips and cursor updates.
AC-4: auto — lint_skill_prose.py forbidden_phrases list contains the four mutation verbs (gh pr comment, gh pr merge, gh pr close, git commit); the linter confirms none appear in the current SKILL.md prose.
AC-5: auto — lint_skill_prose.py plugin/skills/pr-check/SKILL.md exits 0 with no violations.

What worked

The forbidden_phrases mechanism in lint_skill_prose.py made AC-4 a one-line invariant — no script needed to enforce the read-only stance.
Modelling on task-ensure-ready (single-purpose, non-interactive, stdout-marker contract) gave a clean shape with minimal scaffolding.
The first-draft SKILL.md tripped the git commit forbidden phrase exactly where the lint would catch it in CI — the lint is the right shape for this class of mistake.

Friction and automation gaps

AC-3 cannot be exercised without a live open PR — a synthetic-PR fixture (or a --mock-state flag on pr-check that takes a JSON blob in place of gh pr view) would let the cursor-flip half run under the eval harness. Worth a follow-up. → T-BM56-pr-check-mock-state-flag
The forbidden-phrase trap fired naturally from the intro sentence “never makes a git commit” — invariant lint caught it, but a SKILL.md template that pre-seeds “non-interactive and read-only” wording would avoid the false-start. Marginal — not worth its own task.
gh pr view --json statusCheckRollup returns an empty array on PRs without checks; the SKILL.md handles this by simply not matching rule 4. But “no checks” is also a fact worth reporting in the digest. Could be a future verdict refinement (NO-CHECKS) or just a richer reason string in CLEAN. → T-VBFY-pr-check-no-checks-verdict
The /sdlc:pr-check skill isn’t invocable from inside the same task-work session that creates it — would need a /reload-plugins after the worktree’s plugin/skills/pr-check/ exists. Documented friction; out of scope for this task.

Spawned follow-up tasks

T-BM56-pr-check-mock-state-flag — --mock-state flag so AC-3’s cursor-flip half runs deterministically under the eval harness, created.
T-VBFY-pr-check-no-checks-verdict — make “no CI checks configured” distinguishable from “checks passing” in the verdict output, created.

← Back to Tasks