Update Review Checklist

DLC: Update Review Checklist

Cluster recurring reviewer themes across recent merged PRs and open a PR that proposes new entries for the target repo's docs/code-review-checklist.md. The skill never auto-merges — every proposal is human-reviewed.

Standalone: /dlc:update-review-checklist [<owner>/<repo>] [--lookback 30d] [--threshold 2] [--dry-run] [--unattended]

Scheduled (recommended monthly cadence):

/schedule '0 0 1 * *' /dlc:update-review-checklist <owner>/<repo>

Why This Matters

A docs/code-review-checklist.md doc decays without a forcing function. Manual curation lapses after the first few months and the checklist drifts away from what reviewers are actually catching in PRs. Reviewer attention is the most expensive resource on the team — when the same class of finding shows up in two or more PRs, the checklist deserves an entry so the next author catches it before review.

The recurring nature is the entire signal. A one-off finding is noise; a finding that recurs across multiple PRs is a pattern. Monthly cadence gives enough signal to cluster without flooding the checklist with low-recurrence entries. A run that finds zero promotable clusters is a healthy result — it means recent reviews have surfaced novel issues, not the same ones again.

Notification Rules

PushNotification fires only for terminal states that benefit the user knowing about:

• pr_opened — a checklist update PR was opened (include the PR URL)
• pending_human — under --unattended, clusters that need a human call were skipped
• Errors that block the run (missing prerequisites, GitHub API failures)

Routine outcomes (no clusters found, missing checklist file, dry-run with nothing to propose) are silent — printed to stdout but not pushed. This keeps the monthly scheduled cadence quiet by default and noisy only when there is something to act on.

Step 0: Parse Arguments

Capture the original calling directory once, before any later step changes cwd into a clone:

ORIG_CWD="$(pwd)"

Read $ARGUMENTS and extract:

• REPO — optional positional owner/name. If absent, default to gh repo view --json owner,name for the current working directory.
• LOOKBACK — --lookback <Nd> (default 30d). Format is a positive integer followed by d.
• THRESHOLD — --threshold <N> (default 2). Minimum number of distinct PRs a cluster must span to be promoted. Must be a positive integer ≥1.
• DRY_RUN — --dry-run flag. When present, set DRY_RUN=true.
• UNATTENDED — --unattended flag. When present, suppress AskUserQuestion calls; ambiguous clusters and ambiguous dedup matches become Pending-Human items reported in the final summary instead of prompting the user.

Reject unknown flags, non-Nd LOOKBACK values, and non-positive-integer THRESHOLD values with a one-line error and exit non-zero. THRESHOLD validation matters because it is emitted unquoted into the state-file JSON in Step 8 — a malformed value corrupts the file.

Initialise two counters that other steps mutate. PENDING_HUMAN is a JSON array because Step 8 pipes it to jq (jq 'length // 0') and Steps 4/5 mutate it via jq appends — an unset or non-JSON value breaks the pipeline:

PENDING_HUMAN='[]'   # JSON array; append records via `jq '. + [$new]'`
ENTRIES_ADDED=0

• PENDING_HUMAN — JSON array of {theme, source_prs, reason} records (clusters held back from the PR because they needed a human call). Used in Step 9 to emit the Pending-Human: line.
• ENTRIES_ADDED — count of clusters actually written to the checklist file (distinct from "clusters that survived clustering" — Pending-Human clusters are not counted here).

Step 1: Precondition Check

Verify the target repo has docs/code-review-checklist.md. Capture stderr so a 404 (missing file — expected) can be distinguished from real failures (auth, rate limit, network).

First confirm the repo itself is accessible, because gh api returns the same HTTP 404 for "repo not found / no access" as it does for "file not found". Without this disambiguation, an auth or typo failure would be silently treated as "missing checklist" and exit 0 in a scheduled run — masking real problems:

if ! repo_err=$(gh api "repos/$REPO" --silent 2>&1 >/dev/null); then
  printf 'Cannot access repo %s: %s\n' "$REPO" "$repo_err" >&2
  # Caller should fire PushNotification with $repo_err and exit non-zero.
  exit 1
fi

if ! gh_err=$(gh api "repos/$REPO/contents/docs/code-review-checklist.md" --silent 2>&1 >/dev/null); then
  if printf '%s\n' "$gh_err" | grep -q "HTTP 404"; then
    echo "$REPO has no docs/code-review-checklist.md — create one first, then re-run /dlc:update-review-checklist. See issue cc-skills#216 for the shape."
    exit 0
  fi
  # Real failure — auth, rate limit, network. Surface the message and notify.
  printf 'Precondition check failed for %s: %s\n' "$REPO" "$gh_err" >&2
  # Caller should fire PushNotification with $gh_err and exit non-zero.
  exit 1
fi

The HTTP 404 substring match is the documented gh api error format — gh api prints gh: ... (HTTP 404) to stderr on missing resources. The repo-level check above ensures that a 404 reaching the file check is unambiguously "file missing" rather than "repo inaccessible".

Step 1.5: Guard Against an Existing Open Update PR

The --skip-prefix filter in the helper script only excludes merged prior runs. An open prior PR is still in flight; opening a second one stacks duplicates and pollutes the review queue.

OPEN_PRIOR=$(gh pr list --repo "$REPO" --state open --limit 200 \
  --json headRefName,url \
  --jq '[ .[] | select(.headRefName | startswith("chore/update-review-checklist-")) ][0].url // empty')

If OPEN_PRIOR is non-empty:

Print one line to stdout naming the existing PR's URL and exit 0. Do not fire PushNotification — a single waiting PR is a normal between-cycles state, not an error:

$REPO has an open checklist-update PR (<URL>). Merge or close it before re-running.

This rule applies in both attended and unattended modes. Dry-run is the one exception: continue through to Step 6 so the human can preview what would be proposed once the prior PR is resolved.

Step 2: Fetch Comments

Run the helper script that lists merged PRs in window, fetches review-thread + review-body + issue-comment data per PR, applies the resolved-by-commit heuristic, and detects severity labels:

The skill's bash block references the helper script via ${CLAUDE_SKILL_DIR}/../../scripts/fetch-merged-pr-comments.sh so it resolves deterministically regardless of execution cwd.

FETCH_ERR="$(mktemp "${TMPDIR:-/tmp}/update-review-checklist-fetch-err.XXXXXX")"
PR_DATA=$(sh "${CLAUDE_SKILL_DIR}/../../scripts/fetch-merged-pr-comments.sh" "$REPO" --lookback "$LOOKBACK" 2>"$FETCH_ERR") || {
  err_msg=$(jq -r '.error // .' "$FETCH_ERR" 2>/dev/null || cat "$FETCH_ERR")
  rm -f "$FETCH_ERR"
  echo "fetch-merged-pr-comments.sh failed: $err_msg" >&2
  exit 1
}

# Validate response shape — the helper writes errors to stderr (handled above)
# but the success path must still contain the expected JSON keys.
if ! printf '%s' "$PR_DATA" | jq -e '.prs and .summary' >/dev/null; then
  echo "fetch-merged-pr-comments.sh: response missing .prs or .summary" >&2
  rm -f "$FETCH_ERR"
  exit 1
fi

# Replay per-PR warnings (GraphQL skips, jq-transform failures) on stderr when
# the helper succeeded — useful for operators in scheduled runs. The script
# uses stderr for *both* fatal errors (already handled above) and these soft
# warnings; we re-emit them to stderr so they reach the same destination an
# operator would inspect, rather than silently discarding them.
if [ -s "$FETCH_ERR" ]; then
  echo "fetch-merged-pr-comments.sh warnings:" >&2
  cat "$FETCH_ERR" >&2
fi
rm -f "$FETCH_ERR"

# Extract cutoff_date from the helper response — used in the Step 9 summary.
CUTOFF_DATE=$(printf '%s' "$PR_DATA" | jq -r '.cutoff_date')

The helper uses die_json to write {error, code} JSON to stderr and exit non-zero, so validation must check the exit code and parse stderr (not look for .error in $PR_DATA). The jq -e '.prs and .summary' check enforces the documented schema on the success path. The mktemp path mirrors the CURRENT_CHECKLIST pattern below — concurrent scheduled runs against the same $TMPDIR cannot collide on a fixed /tmp/fetch-err.json path.

If .summary.truncated == true: Warn on stdout that the helper hit a per-PR pagination cap and some comments may be missing. Continue with the partial data.

If .summary.list_limit_hit == true: Warn that the merged-PR list cap (200) was reached and recent PRs may be missing from analysis. Continue with what was returned.

Existing checklist read. Also fetch the current checklist content for the dedup step in Step 5. Use mktemp so concurrent scheduled runs against different repos cannot overwrite each other's dedup input:

CURRENT_CHECKLIST="$(mktemp "${TMPDIR:-/tmp}/update-review-checklist-existing.XXXXXX")"
trap 'rm -f "$CURRENT_CHECKLIST"' EXIT  # clean up on any exit path — important for scheduled recurring runs

# Capture gh api output separately so a fetch failure (auth, rate limit, 404)
# is not masked by a successful base64 exit status in a pipeline.
ENCODED=$(gh api "repos/$REPO/contents/docs/code-review-checklist.md" --jq '.content') || {
  echo "Failed to fetch docs/code-review-checklist.md from $REPO" >&2
  exit 1
}

# Portable base64 decode: GNU/macOS Big Sur+ accept --decode; older BSD/macOS
# use -D. Fall back across both so the skill works on every reviewer-supported
# runtime without forcing a coreutils dependency. Fail fast if BOTH fail —
# otherwise Step 5 would dedup against an empty/corrupt checklist and propose
# duplicates of entries that already exist.
if ! printf '%s' "$ENCODED" | base64 --decode > "$CURRENT_CHECKLIST" 2>/dev/null; then
  if ! printf '%s' "$ENCODED" | base64 -D > "$CURRENT_CHECKLIST" 2>/dev/null; then
    echo "Failed to base64-decode docs/code-review-checklist.md from $REPO (neither --decode nor -D succeeded)" >&2
    exit 1
  fi
fi

Step 3: Filter Comments

Apply these filters in order; keep counters so the final summary can report what was dropped and why.

1. Drop unresolved-by-commit (.resolved_by_commit == false). A reviewer comment that did not provoke a code change from the PR author is either (a) a wishlist item ignored or deferred, or (b) something the reviewer themselves resolved as not-needed. Either way, it is not a pattern worth promoting to the checklist.
2. Drop hard-skip patterns — read references/clustering-rubric.md now and apply its explicit blocklist (typos, formatting nits, "consider" wishlists with no specific action, automated review-summary comments with zero findings).

The .is_bot field on each comment is retained as metadata for the PR body's "Derived from" table, but is not a filter input. Substantive reviewers in many repos are bot accounts (Copilot, CodeRabbit, Codex, Gemini, Qodo); filtering on author type would discard the primary signal. Noise from automated reviewers is content-defined and handled by the hard-skip patterns instead.

Record the per-step drop counts. They surface in the Step 9 summary.

Step 4: Cluster Semantically

Read references/clustering-rubric.md now and apply its semantic rules to group the surviving comments into clusters.

A cluster's PR-count is the number of distinct PR numbers its member comments come from — not the total comment count. Two comments from the same PR count once.

A cluster's weight is the sum of member severity weights from the rubric (high=3, medium=2, low|null=1).

Drop any cluster with pr_count < $THRESHOLD. Sort the survivors by weight descending, then by PR-count descending.

Ambiguous-cluster gate

After applying the rubric, mark a cluster as ambiguous when any of the following hold:

• Members straddle the "same ask, different domain" line in the rubric (e.g. "add a test" for both an algorithm and a UI component) — could legitimately split into two clusters
• The cluster's proposed title can be read as either too coarse ("Improve error handling") or genuinely useful, and you cannot decide from the member comments alone
• Two existing clusters could merge into one broader cluster, or one cluster could split into two narrower ones, and the rubric's granularity guide doesn't break the tie

For each ambiguous cluster:

• If UNATTENDED == true: append {theme, source_prs, reason: "ambiguous_clustering"} to PENDING_HUMAN and drop the cluster from the active set. Pending-Human items are surfaced in Step 9, not in the PR.

• If UNATTENDED == false: invoke AskUserQuestion once per ambiguous cluster with options shaped like:

  Cluster "<theme>" spans PRs #N, #M, #P. Members suggest <split/merge/keep>. Which?
    (a) Keep as-is and propose
    (b) Split into <theme-a> and <theme-b>
    (c) Drop (too ambiguous to be useful)
  

  Apply the user's answer to the active set before continuing.

Non-ambiguous clusters pass through unchanged. The active set after this gate is what Step 5 sees.

Step 5: Dedup Against Existing Checklist

Read references/checklist-schema.md now. For each surviving cluster, apply its semantic-dedup rubric against the checklist content at $CURRENT_CHECKLIST (the per-run path from Step 2). The rubric's three diagnostic questions classify each cluster as duplicate (drop), distinct (keep), or ambiguous (needs a call).

Ambiguous-dedup gate

A cluster is ambiguous against an existing entry when Q1/Q2/Q3 produce a mix of "maybe" answers — typically because the existing entry is generic and the cluster is specific (Q2 subsumption is unclear), or because the surface partially overlaps (Q3 partial-domain match).

For each ambiguous dedup match:

• If UNATTENDED == true: append {theme, source_prs, reason: "ambiguous_dedup_vs_existing:<existing-entry-title>"} to PENDING_HUMAN and drop the cluster from the active set.

• If UNATTENDED == false: invoke AskUserQuestion:

  Cluster "<theme>" (PRs #N, #M) may overlap with existing entry "<E>". Propose anyway?
    (a) Yes — distinct enough, add to PR
    (b) No — covered by "<E>"
    (c) Replace "<E>" with the new wording  [INFORMATIONAL — out of scope for v1; treat as (b) but record]
  

  Option (c) is out of scope per issue cc-skills#216 (no rewording of existing entries in v1) — record the suggestion in the PR body for the human reviewer's consideration, but treat behaviour as (b) and drop the cluster.

Tie-breaker for genuine doubt that isn't ambiguous (attended mode without an AskUserQuestion prompt): prefer keeping the candidate. Duplicate proposals get caught in human review; missed proposals do not. The ambiguous-dedup gate above only fires for the specifically ambiguous cases — most clusters will pass cleanly as either duplicate or distinct.

Step 6: Dry-Run Exit Point

If DRY_RUN=true:

Print each surviving cluster to stdout in this format:

Cluster: <theme>
  PRs:      #N, #M, #P  (n distinct, weight=W)
  Members:  <reviewer1> on #N: "<comment excerpt 80c>"
            <reviewer2> on #M: "<comment excerpt 80c>"
  Proposed entry:
    > <entry text per checklist-schema.md format>
    > Source PRs: #N, #M, #P

Then print a summary block: total clusters proposed, filter drop counts from Step 3, dedup drop count from Step 5.

Do not create a branch, edit any file, or open a PR. Exit 0.

Step 7: Author the PR

If DRY_RUN=false and at least one cluster survives the dedup gate (i.e. the active set is non-empty after Step 5):

Always operate in a fresh clone

Use a temporary work directory unconditionally — even when the target repo is the current working directory. This avoids two-path branching, prevents accidental edits in the user's working tree, and keeps the calling repo's git state clean for state-file writes in Step 8:

WORKDIR="$(mktemp -d "${TMPDIR:-/tmp}/update-review-checklist.XXXXXX")"
# Compose with the Step 2 cleanup so both temp resources are removed. Bash
# `trap '...' EXIT` REPLACES the previous EXIT trap, so a bare
# `trap 'rm -rf "$WORKDIR"' EXIT` here would silently strand the checklist
# tempfile created in Step 2. Compose both cleanups into one handler.
trap 'rm -rf "$WORKDIR"; rm -f "$CURRENT_CHECKLIST"' EXIT

BRANCH="chore/update-review-checklist-$(date -u +%Y-%m-%d-%H%M%S)"

gh repo clone "$REPO" "$WORKDIR" -- --depth 50 >/dev/null
cd "$WORKDIR"
git checkout -b "$BRANCH"

The trap ... EXIT ensures the work dir is cleaned up whether the step succeeds, fails, or is interrupted.

Re-read the checklist from the clone

The $CURRENT_CHECKLIST cache from Step 2 may be stale relative to the freshly cloned HEAD. Re-read in-place so Step 5's dedup is sound against the actual file you're about to edit:

CHECKLIST_PATH="$WORKDIR/docs/code-review-checklist.md"

If a meaningful diff exists between $CURRENT_CHECKLIST and $CHECKLIST_PATH (e.g. someone merged checklist changes between Step 2 and now), re-run the Step 5 dedup gate against the fresh content before continuing. For monthly-cadence runs this is almost never the case.

Edit the checklist

Apply the active-set entries per references/checklist-schema.md — match each cluster to its target section (or append to a "Recurring patterns" section if no existing match), and include the > Source PRs: #N, #M trailer on every entry. Use Edit operations rather than rewriting the file. Each successful edit increments ENTRIES_ADDED.

Commit, push, open PR

GPG-signing must be bypassed — scheduled / unattended runs have no interactive pinentry available:

git add docs/code-review-checklist.md
git -c commit.gpgsign=false commit -m "chore(dlc): update review checklist (lookback $LOOKBACK)"
git push -u origin "$BRANCH"

Build the PR body per the template in references/checklist-schema.md.

No GitHub mentions in posted PR body: Any text this skill posts to GitHub — including the "Derived from" table, "Pending human review" section, and summary prose — MUST NOT contain an @-prefixed username or bot name. Write bare logins/names (e.g. ana, ben, copilot) without @ prefixes to avoid triggering unintended mention notifications or bot actions. If quoting reviewer comments or technical content containing an @ character (such as scoped packages or decorators), insert a space immediately after the @ (e.g. @ name).

Include:

• A one-line summary of ENTRIES_ADDED
• A "Derived from" table listing each new entry and the PRs it was clustered from (acceptance criterion #4)
• The lookback window and threshold used
• If PENDING_HUMAN is non-empty, a separate "Pending human review" section listing the held-back themes with their source_prs and reason — informational, so reviewers know these clusters were observed but not promoted (no @-mentions)
• A "How this was generated" footer pointing at this skill

if grep -qE '@[[:alnum:]_-]' "$WORKDIR/.pr-body.md"; then
  echo "ERROR: PR body contains unneutralized @-mentions. Refusing to post." >&2
  exit 1
fi

gh pr create \
  --repo "$REPO" \
  --title "chore(dlc): update review checklist (lookback $LOOKBACK)" \
  --body-file "$WORKDIR/.pr-body.md"

Capture the PR URL into PR_URL. Return to the original cwd before Step 8 so state-file writes target the calling repo, not the clone:

cd "$ORIG_CWD"

Step 8: Persist State

Write a small JSON state file in the calling repo (the place this skill was invoked from), not the cloned target. This file is informational; it does not gate future runs.

Pick STATUS and LAST_PR_URL based on what actually happened, so the state file truthfully reflects the run instead of always claiming pr_opened:

• pr_opened — DRY_RUN=false, a PR was created, $PR_URL is set
• no_entries — DRY_RUN=false but no clusters survived dedup (Step 7 was a no-op); $PR_URL empty
• dry_run — DRY_RUN=true (Step 6 exit path); $PR_URL empty
• pending_human_only — only Pending-Human clusters; no PR opened; $PR_URL empty

SLUG=$(printf '%s' "$REPO" | tr '/' '-')
mkdir -p "$ORIG_CWD/.dev/dlc"
PENDING_HUMAN_COUNT=$(printf '%s' "$PENDING_HUMAN" | jq 'length // 0')

if [ "$DRY_RUN" = "true" ]; then
  STATUS="dry_run"; LAST_PR_URL=""
elif [ -n "$PR_URL" ]; then
  STATUS="pr_opened"; LAST_PR_URL="$PR_URL"
elif [ "$PENDING_HUMAN_COUNT" -gt 0 ]; then
  STATUS="pending_human_only"; LAST_PR_URL=""
else
  STATUS="no_entries"; LAST_PR_URL=""
fi

cat > "$ORIG_CWD/.dev/dlc/update-review-checklist-$SLUG.state" <<EOF
{
  "last_run_at": "$(date -u +%Y-%m-%dT%H:%M:%SZ)",
  "last_pr_url": "$LAST_PR_URL",
  "lookback": "$LOOKBACK",
  "threshold": $THRESHOLD,
  "entries_added": $ENTRIES_ADDED,
  "pending_human": $PENDING_HUMAN_COUNT,
  "status": "$STATUS"
}
EOF

The state file lives alongside babysit's (.dev/dlc/*.state). Writing it to $ORIG_CWD matters because Step 7's clone is in a temp dir that gets rm -rf'd on exit — anything written there is lost.

Why no self-cancel. Unlike dlc:babysit, this skill is recurring by design — every month may yield new clusters. Do not call CronDelete to remove the schedule entry. Under /loop usage (ad-hoc, not the recommended cadence), the user manages stop via /loop stop themselves.

Step 9: Terminal Notification + Summary

Emit a single summary block to stdout. The two key counters are deliberately separated so the user can see at a glance "how many entries actually landed in the PR" vs "how many clusters were observed but held back":

update-review-checklist complete.
  Repo:                 $REPO
  Lookback:             $LOOKBACK (cutoff $CUTOFF_DATE)
  Merged PRs inspected: $N  (skipped own PRs: $K)
  Comments fetched:     $M
  Filter drops:
    - unresolved-by-commit:       $B
    - hard-skip patterns:         $C
  Clusters above threshold:       $D
  Dedup drops (already covered):  $E
  Entries added to PR:            $ENTRIES_ADDED
  Pending-Human (held back):      $PENDING_HUMAN_COUNT  [only printed when > 0]
  PR opened:                      $PR_URL                [only printed when a PR was opened]

ENTRIES_ADDED counts clusters that landed in the PR. PENDING_HUMAN_COUNT counts ambiguous clusters held back via the Step 4 or Step 5 gates under --unattended. The two sets are disjoint by construction — Pending-Human items are dropped from the active set before Step 7 edits the file.

Notifications

• If a PR was opened (ENTRIES_ADDED > 0 and not dry-run): fire PushNotification with message Review-checklist PR opened for $REPO: $ENTRIES_ADDED new entries (lookback $LOOKBACK). $PR_URL
• If --unattended and PENDING_HUMAN_COUNT > 0: fire PushNotification with message Review checklist: $PENDING_HUMAN_COUNT clusters need a human call for $REPO. Re-run attended to triage.
• If ENTRIES_ADDED == 0 and not dry-run: silent. No PR, no push. The summary above is enough — zero clusters worth promoting is a healthy result for a well-reviewed repo.
• Dry-run: silent regardless of counts. The user invoked dry-run because they want stdout, not a push.

Step 10: Verify Against Acceptance Criteria

Cross-reference the run against the VERIFY criteria in issue cc-skills#216: