feat(review): gate prompt sections by review cycle

Author: zfarrell

.github/workflows/tests.yml

@@ -18,3 +18,6 @@ jobs:
 
       - name: Review cycle counter
         run: tests/review-cycle-test.sh
+
+      - name: Cycle-gated prompt rendering
+        run: tests/render-prompt-test.sh

docs/claude-pr-review-prompt.md

@@ -4,41 +4,40 @@ You are an expert code reviewer embedded in a GitHub Actions workflow. Your job
 
 This prompt includes:
 - **REVIEW CYCLE** — which review iteration this is (1 = first review, 2+ = re-review after changes)
-- **Prior Review Comments** — existing inline comment threads from previous reviews, including author responses. If this is cycle 1, there will be no prior comments — skip straight to reviewing the code.
+- **Prior Review Comments** — existing inline comment threads from previous reviews, including author responses
+<!-- cycle>=2 -->
+- **`<incremental_diff>`** — the changes pushed since your last review. This is your review surface for this cycle.
+<!-- /cycle -->
+
+This prompt has already been narrowed to the current review cycle by the workflow. Everything below applies as written — there is no cycle ladder left for you to interpret, and nothing here is conditional on the cycle number.
 
 ## Review Process
 
-1. **Understand the PR** — read the title, description, and linked issues to understand intent
-2. **Read prior review threads** — if cycle 2+, read the prior review comments included above to understand what feedback was already given and how the author responded
-3. **Inspect the diff** — use `gh pr diff` to see what changed
-4. **Read affected files** — use `Read` to get full context around changed code
-5. **Post feedback** — use inline comments for specific issues, and a summary comment only when requesting changes
+Work in this order:
 
-## Handling Prior Feedback (cycle 2+ only)
+- **Understand the PR** — read the title, description, and linked issues to understand intent
+<!-- cycle==1 -->
+- **Inspect the diff** — use `gh pr diff` to see what changed
+<!-- /cycle -->
+<!-- cycle>=2 -->
+- **Read prior review threads** — understand what feedback was already given and how the author responded
+- **Inspect only the new changes** — the `<incremental_diff>` block above holds everything pushed since your last review. That is your review surface. Do not re-review code you already passed on, and do not mine unchanged hunks for findings you missed the first time. Fall back to `gh pr diff` only if that block reports the incremental diff is unavailable.
+<!-- /cycle -->
+- **Read affected files** — use `Read` to get full context around changed code
+- **Post feedback** — inline comments for specific issues; a summary comment only when requesting changes
 
-Skip this section entirely on cycle 1.
+<!-- cycle>=2 -->
+## Handling Prior Feedback
 
-- **Blocking issues stay blocking.** If a prior review flagged a blocking issue, it remains blocking until it is fixed in the code. An author reply alone does not resolve a blocking issue — the code must change. If the author's reply reveals that your original assessment was wrong (e.g., you misread the code), you may drop it.
+- **Blocking issues stay blocking.** If a prior review flagged a blocking issue, it remains blocking until it is fixed in the code. An author reply alone does not resolve a blocking issue — the code must change. If the author's reply reveals that your original assessment was wrong (e.g. you misread the code), you may drop it.
 - **Do not re-raise resolved issues.** If prior blocking feedback was addressed in new commits, move on.
 - **Do not re-raise nits the author declined.** If the author pushed back on a non-blocking suggestion with a reasonable explanation, respect their judgment and do not repeat it.
-- If all prior blocking issues are resolved, update your review status accordingly (approve or request changes based on new findings only).
-
-## Review Cycle Awareness
-
-- **Cycle 1–2**: Full review. Flag blocking issues and nits.
-- **Cycle 3–4**: Focus on blocking issues. Only leave nits if they are genuinely important.
-- **Cycle 5+**: Blocking issues only. Do not leave any nits.
-
-The goal is to converge toward merge, not to find new things to complain about in each round.
+- **Do not charge the author for churn you caused.** Code comments and docstrings that drifted out of date because the author was addressing your earlier feedback are not new findings. Leave them alone.
+- If all prior blocking issues are resolved, update your review status based on new findings only.
+<!-- /cycle -->
 
 ## Review Criteria
 
-### Code Quality
-- Follows existing style and conventions in the repo
-- No commented-out code or debug artifacts
-- Meaningful, consistent naming
-- DRY — no unnecessary duplication
-
 ### Correctness
 - No obvious bugs or off-by-one errors
 - Edge cases are handled
@@ -60,10 +59,21 @@ The goal is to converge toward merge, not to find new things to complain about i
 - No obvious N+1 queries or unnecessary loops
 - No blocking calls in hot paths
 
+<!-- cycle<=2 -->
+### Code Quality
+- Follows existing style and conventions in the repo
+- No commented-out code or debug artifacts
+- Meaningful, consistent naming
+- DRY — no unnecessary duplication
+<!-- /cycle -->
+
+<!-- cycle==1 -->
 ### Documentation
 - Public APIs and functions are documented
 - README or docs updated if user-facing behavior changed
+<!-- /cycle -->
 
+<!-- cycle<=2 -->
 ## Severity Classification
 
 Classify all findings into one of three levels:
@@ -77,13 +87,35 @@ Classify all findings into one of three levels:
 - **No issues found** → approve with `gh pr review --approve`. No summary comment.
 - **Only nits/super nits** → approve with `gh pr review --approve`. Leave inline comments. No summary comment.
 - **Blocking issues found** → request changes with `gh pr review --request-changes`. Leave inline comments. Leave a summary comment (format below).
+<!-- /cycle -->
+
+<!-- cycle>=3 -->
+## What to Report
+
+Report **blocking issues only**: security vulnerabilities, data loss, broken builds, correctness bugs, logic errors, missing error handling on critical paths, race conditions.
+
+Everything else is out of scope for this cycle. That includes code quality, naming, duplication, style, documentation and comment gaps, test-shape preferences, and every other non-blocking observation. Do not post it — not as an inline comment, not as an observation or heads-up or FYI, and not as a parenthetical tacked onto a blocking comment.
+
+If the only things you found are non-blocking, approve with no inline comments at all.
+
+The author has already been through several rounds on this PR. Converging is worth more than completeness. A finding you could have raised on cycle 1 and did not is not worth raising now.
+
+## Decision Framework
+
+- **No blocking issues** → approve with `gh pr review --approve`. No inline comments. No summary comment.
+- **Blocking issues found** → request changes with `gh pr review --request-changes`. Leave inline comments for each. Leave a summary comment (format below).
+<!-- /cycle -->
 
 ## Output Rules
 
 - **Do not be chatty.** No filler, no praise, no "looks good overall" preamble.
 - **Do not feel compelled to find problems.** If the code is fine, approve it.
 - **Do not nitpick.** Skip style issues that a linter should catch.
+<!-- cycle<=2 -->
 - Nit and super nit comments MUST always include `(not blocking)`.
+<!-- /cycle -->
+- Be direct and specific — cite file paths and line numbers
+- Be constructive — explain *why* something is a problem and suggest a fix
 - Only leave a summary comment when requesting changes:
 
 ```
@@ -95,6 +127,3 @@ Classify all findings into one of three levels:
 ### Action Required
 [Specific changes needed before this can merge]
 ```
-
-- Be direct and specific — cite file paths and line numbers
-- Be constructive — explain *why* something is a problem and suggest a fix

scripts/render-prompt.sh

@@ -0,0 +1,72 @@
+#!/usr/bin/env bash
+#
+# Renders the review prompt for one specific review cycle, on stdout, from the prompt on
+# stdin. Blocks wrapped in cycle markers are kept or dropped here, before the prompt ever
+# reaches the model:
+#
+#   <!-- cycle==1 -->  ... <!-- /cycle -->   kept only on cycle 1
+#   <!-- cycle<=2 -->  ... <!-- /cycle -->   kept on cycles 1-2
+#   <!-- cycle>=3 -->  ... <!-- /cycle -->   kept on cycle 3 and later
+#
+# Why this is a script and not three sentences in the prompt: the prompt used to carry a
+# "Review Cycle Awareness" ladder telling the model to taper nits as cycles climbed, and
+# measurement showed it was ignored outright. Across cycle 6+ reviews the ladder's rule was
+# "blocking issues only, do not leave any nits" and the actual output was 31 comments, 0 of
+# them blocking, 100% nits. Advisory self-restraint loses to a 50-line criteria checklist
+# that invites finding things. A block that is never sent cannot be ignored.
+#
+# Markers must sit alone on their own line and must not nest. Malformed markers are a hard
+# error (exit 2) rather than a silent mis-render; the caller falls back to the full prompt.
+
+set -euo pipefail
+
+if [ $# -ne 1 ]; then
+  echo "usage: render-prompt.sh <review-cycle> < prompt.md" >&2
+  exit 2
+fi
+
+case $1 in
+  '' | *[!0-9]*)
+    echo "render-prompt.sh: review cycle must be a positive integer, got '$1'" >&2
+    exit 2
+    ;;
+esac
+
+if [ "$1" -lt 1 ]; then
+  echo "render-prompt.sh: review cycle must be >= 1, got '$1'" >&2
+  exit 2
+fi
+
+# `cat -s` collapses the run of blank lines a dropped block leaves behind. pipefail keeps
+# awk's exit status, so a malformed marker still fails the pipeline.
+awk -v C="$1" '
+BEGIN { depth = 0; keep = 1; err = "" }
+
+/^<!-- cycle(<=|>=|==)[0-9]+ -->$/ {
+  if (depth) { err = "nested cycle block at line " NR; exit 2 }
+  spec = $0
+  sub(/^<!-- cycle/, "", spec)
+  sub(/ -->$/, "", spec)
+  op = substr(spec, 1, 2)
+  n = substr(spec, 3) + 0
+  depth = 1
+  if (op == "<=")      keep = (C <= n)
+  else if (op == ">=") keep = (C >= n)
+  else                 keep = (C == n)
+  next
+}
+
+/^<!-- \/cycle -->$/ {
+  if (!depth) { err = "unmatched <!-- /cycle --> at line " NR; exit 2 }
+  depth = 0
+  keep = 1
+  next
+}
+
+{ if (!depth || keep) print }
+
+END {
+  if (err != "") { printf "render-prompt.sh: %s\n", err > "/dev/stderr"; exit 2 }
+  if (depth)     { printf "render-prompt.sh: unclosed cycle block\n" > "/dev/stderr"; exit 2 }
+}
+' | cat -s

tests/render-prompt-test.sh

@@ -0,0 +1,162 @@
+#!/usr/bin/env bash
+#
+# Guards the mechanical cycle gating in scripts/render-prompt.sh, and asserts the shipped
+# prompt actually renders the way each cycle needs it to. These run against the real
+# docs/claude-pr-review-prompt.md, not a fixture, so editing the prompt into a state where
+# a late cycle can emit nits again fails the suite.
+#
+# Regression this exists for: the prompt used to carry a "Review Cycle Awareness" ladder
+# ("Cycle 5+: blocking issues only. Do not leave any nits.") and the model ignored it. Over
+# the cycle 6+ reviews sampled, output was 31 comments, 0 blocking, 100% nits. The gating is
+# mechanical now -- the nit taxonomy is not in the cycle 3+ prompt at all -- so these tests
+# check for the absence of text rather than trusting a rule to be obeyed.
+
+set -euo pipefail
+
+cd "$(dirname "$0")/.."
+
+RENDER=scripts/render-prompt.sh
+PROMPT=docs/claude-pr-review-prompt.md
+
+failures=0
+
+pass() { echo "ok   $1"; }
+fail() {
+  echo "FAIL $1"
+  failures=$((failures + 1))
+}
+
+render() { bash "$RENDER" "$1" < "$PROMPT"; }
+
+# has <cycle> <pattern> <description>
+has() {
+  if render "$1" | grep -qF -- "$2"; then pass "cycle $1 keeps: $3"; else fail "cycle $1 is missing: $3"; fi
+}
+
+# lacks <cycle> <pattern> <description>
+lacks() {
+  if render "$1" | grep -qF -- "$2"; then fail "cycle $1 still contains: $3"; else pass "cycle $1 drops: $3"; fi
+}
+
+echo "-- marker mechanics --"
+
+# Each comparison operator, on both sides of its boundary.
+fixture=$(mktemp)
+trap 'rm -f "$fixture"' EXIT
+cat > "$fixture" <<'EOF'
+always
+<!-- cycle==1 -->
+only-one
+<!-- /cycle -->
+<!-- cycle<=2 -->
+upto-two
+<!-- /cycle -->
+<!-- cycle>=3 -->
+three-plus
+<!-- /cycle -->
+tail
+EOF
+
+# expect_render <cycle> <space separated expected lines>
+expect_render() {
+  local cycle=$1 want=$2 got
+  got=$(bash "$RENDER" "$cycle" < "$fixture" | tr '\n' ' ' | sed 's/  */ /g;s/ $//')
+  if [ "$got" = "$want" ]; then
+    pass "cycle $cycle renders '$want'"
+  else
+    fail "cycle $cycle: expected '$want', got '$got'"
+  fi
+}
+
+expect_render 1 "always only-one upto-two tail"
+expect_render 2 "always upto-two tail"
+expect_render 3 "always three-plus tail"
+expect_render 9 "always three-plus tail"
+
+# Markers themselves must never reach the model.
+if bash "$RENDER" 1 < "$fixture" | grep -q '<!-- '; then
+  fail "marker comments leak into the rendered output"
+else
+  pass "marker comments are stripped from the output"
+fi
+
+echo "-- input validation --"
+
+# expect_reject <args...> -- the script must exit 2, not silently mis-render
+expect_reject() {
+  local desc=$1; shift
+  if bash "$RENDER" "$@" < "$fixture" >/dev/null 2>&1; then
+    fail "accepted invalid input: $desc"
+  else
+    pass "rejects $desc"
+  fi
+}
+expect_reject "a non-numeric cycle" abc
+expect_reject "cycle 0" 0
+expect_reject "a negative cycle" -1
+expect_reject "no arguments"
+
+# Malformed markers are a hard error so the caller can fall back to the full prompt rather
+# than ship a half-stripped one.
+for bad in '<!-- cycle>=2 -->\nx\n<!-- cycle>=3 -->\ny\n<!-- /cycle -->' \
+           '<!-- cycle>=2 -->\nx' \
+           'x\n<!-- /cycle -->'; do
+  if printf '%b\n' "$bad" | bash "$RENDER" 2 >/dev/null 2>&1; then
+    fail "accepted malformed markers: $bad"
+  else
+    pass "rejects malformed markers: $bad"
+  fi
+done
+
+echo "-- shipped prompt, cycle 1 (full review) --"
+has 1 "### Documentation" "the Documentation criterion"
+has 1 "### Code Quality" "the Code Quality criterion"
+has 1 "## Severity Classification" "the nit taxonomy"
+has 1 'gh pr diff' "the full-diff instruction"
+lacks 1 "## Handling Prior Feedback" "prior-feedback handling (nothing to handle yet)"
+lacks 1 "incremental_diff" "the incremental diff reference"
+
+echo "-- shipped prompt, cycle 2 (focused review) --"
+# Documentation drops at cycle 2+: doc/comment drift was 42% of late-cycle comments, and most
+# of it is churn the review loop created by asking the author to change the code underneath.
+lacks 2 "### Documentation" "the Documentation criterion"
+has 2 "## Severity Classification" "the nit taxonomy (nits are still useful at cycle 2)"
+has 2 "## Handling Prior Feedback" "prior-feedback handling"
+has 2 "incremental_diff" "the incremental diff reference"
+has 2 "Do not charge the author for churn you caused" "the churn guard"
+
+echo "-- shipped prompt, cycle 3+ (blocking only) --"
+for c in 3 6 12; do
+  lacks "$c" "## Severity Classification" "the nit taxonomy"
+  lacks "$c" "super nit" "the super nit vocabulary"
+  lacks "$c" "### Documentation" "the Documentation criterion"
+  lacks "$c" "### Code Quality" "the Code Quality criterion"
+  has "$c" "Report **blocking issues only**" "the blocking-only instruction"
+  has "$c" "incremental_diff" "the incremental diff reference"
+done
+
+# The single most important assertion in this file: at a late cycle there must be no
+# instruction anywhere that tells the model how to format a non-blocking comment. If a
+# future edit reintroduces one, the 100%-nits behaviour comes back.
+if render 5 | grep -qE 'nit:|\(not blocking\)'; then
+  fail "cycle 5 prompt still explains how to write a nit"
+else
+  pass "cycle 5 prompt has no nit format to follow"
+fi
+
+# Exactly one Decision Framework must survive at every cycle -- two would leave the model
+# choosing between contradictory approve/request-changes rules.
+for c in 1 2 3 7; do
+  n=$(render "$c" | grep -c '^## Decision Framework$' || true)
+  if [ "$n" -eq 1 ]; then
+    pass "cycle $c has exactly one Decision Framework"
+  else
+    fail "cycle $c has $n Decision Framework sections, expected 1"
+  fi
+done
+
+if [ "$failures" -ne 0 ]; then
+  echo "$failures test(s) failed"
+  exit 1
+fi
+echo "all tests passed"