feat(pr-review): shadow-mode quality gate (#185 Phase 2)

[cbeaulieu-gt]

Summary

The prose-regex quality gate (claude-pr-review/quality-gate) is fragile: it can miss blocking findings when the persona phrases them in ways the regex doesn't catch. Issue #223 documents the concrete regression: PR #222's review contained ### Critical Issues Found 🚨 and **two critical inconsistencies** in lowercase — the gate emitted a false success.

Phase 1 (#228/#229) patched the persona to emit a structured JSON marker block at the end of every review. This PR adds the shadow-mode gate that parses that marker and runs alongside the existing prose-regex gate — advisory, never blocking merge.

What ships

• pr-review/lib/parse-marker.sh — sourceable bash parser, single source of truth. Extracts the <!-- claude-pr-review-summary-v1 block from a review body, validates the JSON schema (closed schema: schemaVersion + findings.{critical,high,medium,low}), and emits outcome|critical|high|medium|low on stdout. Four outcomes: marker_missing, marker_invalid, clean, blocking.

• pr-review/tests/marker-cases/ — 5-fixture corpus:

  • pr-222-critical-issues-found.md — the actual PR docs: refresh consumer-facing README for Phase 5+ container-pinned workflows #222 review body (verbatim from the GitHub API). Pre-Phase-1 body, no marker block → expected marker_missing. This is the quality-gate reports success on review with explicit "two critical" language and "### Critical Issues Found" heading #223 regression reproducer.
  • clean-review.md — all-zero findings, Verdict: APPROVE, valid marker → expected clean|0|0|0|0.
  • mixed-severity.md — one finding at each severity level, valid marker with counts → expected blocking|1|1|1|1.
  • marker-missing.md — pre-Phase-1 era body with no marker block → expected marker_missing.
  • marker-malformed.md — marker block present but findings.critical: "two" (string instead of int) → expected marker_invalid. Defect documented inline in the fixture.

• pr-review/tests/run-marker-cases.sh — test driver modeled on claude-command-router/tests/run-cases.sh. set -uo pipefail (not -e), accumulates failures, prints summary: N/M passed, exits 1 on any failure.

• .github/workflows/test.yml — adds a second job marker-cases that runs bash ./pr-review/tests/run-marker-cases.sh on every PR and push to main. Mirrors the existing router-cases job structure.

• .github/workflows/marker-emission-aggregate.yml — weekly cron (Mondays 07:00 UTC) + workflow_dispatch. Queries the last 7 days of merged PRs, checks each head SHA for quality-gate-shadow statuses, counts outcomes (agree:clean, agree:blocking, disagree:*, marker_missing, marker_invalid), writes a step summary table, and opens/updates a deduped issue titled Quality-gate shadow: marker_missing events detected if any marker_missing events occurred. Always exits 0 — observability only.

• pr-review/action.yml — adds one new step Quality gate — structured marker (advisory shadow) after the existing authoritative gate step. The step sources parse-marker.sh via $GITHUB_ACTION_PATH, re-runs the prose-regex (same pattern as the authoritative gate), derives an agree/disagree/marker_missing label, posts claude-pr-review/quality-gate-shadow as success/failure/error, and writes a step-summary markdown table. The existing Quality gate — post claude-pr-review/quality-gate status step is unchanged.

Test plan

• actionlint .github/workflows/test.yml .github/workflows/marker-emission-aggregate.yml — clean locally
• bash ./pr-review/tests/run-marker-cases.sh passes with summary: 5/5 passed in CI (the marker-cases job in this PR's check list)
• After merge, the next PR review run shows both claude-pr-review/quality-gate and claude-pr-review/quality-gate-shadow in the PR checks list
• Shadow status description identifies agree/disagree explicitly (e.g. agree:clean, disagree:prose=blocking/marker=clean, marker_missing)
• Branch protection ruleset NOT updated to require claude-pr-review/quality-gate-shadow — verify in repo Settings → Branches → branch protection rules after merge

Advisory-only guarantee

The shadow status claude-pr-review/quality-gate-shadow MUST NOT be added to branch protection. The authoritative gate is claude-pr-review/quality-gate. The shadow period exists to accumulate evidence for the Phase 3 cutover criterion (locked decision #5: ≥20 reviews across ≥5 PRs, zero disagreement, zero marker_missing).

Refs #185 #183 #223 #227

🤖 Generated by Claude Code on behalf of @cbeaulieu-gt

[github-actions]

PR Review in Progress

I'll review this PR comprehensively across code quality, security, performance, test coverage, and documentation.

Todo List

• Fetch full PR diff
• Read and analyze core implementation (parse-marker.sh)
• Review composite action changes (pr-review/action.yml)
• Review test infrastructure (fixtures, test runner)
• Review CI workflow (marker-emission-aggregate.yml)
• Evaluate security considerations
• Assess test coverage
• Check documentation and clarity
• Post comprehensive review feedback

View job run

[github-actions]

Claude finished @cbeaulieu-gt's task in 1m 49s —— View job

---

PR Review Complete ✅

I've reviewed the incremental changes (c645b45 → 983179e) — these are high-quality cleanup changes addressing actionlint/shellcheck warnings in the aggregator workflow.

Todo List

• Fetch incremental diff (c645b45..983179e)
• Assess if foundational code is touched — No: only workflow lint fixes, reviewing incrementally
• Analyze the workflow changes
• Evaluate security considerations
• Check for any introduced issues
• Post comprehensive review feedback

---

Summary

Changes reviewed: .github/workflows/marker-emission-aggregate.yml

The commit addresses three categories of linter warnings:

1. Added shellcheck disable comments (lines 42, 127, 159) — Correctly suppresses SC2016 warnings for GitHub Actions template expressions (${{ }}) inside shell scripts. These expressions are processed by the Actions runtime before the shell executes, so the warning is a false positive.

2. Removed unused variable (line 85) — The state variable was extracted from the shadow status JSON but never used. Only the description field matters for the outcome classification. Good cleanup.

3. Refactored GITHUB_ENV exports (lines 114-124) — Changed from multiple individual appends to a single grouped block:

   # Before:
   echo "VAR=value" >> "$GITHUB_ENV"
   echo "VAR2=value" >> "$GITHUB_ENV"
   
   # After:
   {
     echo "VAR=value"
     echo "VAR2=value"
   } >> "$GITHUB_ENV"

   This is both more efficient (fewer file operations) and follows shellcheck best practices.

Code Quality ✅

• Clean, focused changes
• Follows CLAUDE.md conventions for shellcheck directives
• No functionality changes — purely lint/style improvements

Security ✅

• No new security concerns introduced
• Grouped export pattern is actually safer (atomic append operation)
• All shellcheck disables are appropriate and documented

Performance ✅

• Minor improvement from grouped exports (7 individual file appends → 1 grouped append)

Test Coverage ✅

• No test changes needed — these are non-functional cleanup changes
• The workflow behavior is unchanged

Documentation ✅

• Shellcheck disable comments are inline (standard practice)
• Commit message clearly states intent

---

Recommendation: APPROVE ✅

These changes successfully address linter warnings without altering workflow behavior. The cleanup improves code quality and follows repository conventions. No issues found.

[github-actions]

Claude finished @cbeaulieu-gt's task in 1m 16s —— View job

---

PR Review Complete ✅

I've reviewed the latest incremental change (983179e → 81bd020).

Todo List

• Fetch incremental diff (983179e → 81bd020)
• Assess if foundational code is touched — No: shellcheck directive placement fix, reviewing incrementally
• Analyze the changes
• Evaluate code quality, security, performance, test coverage, documentation
• Post comprehensive review feedback

---

Summary

Commit reviewed: 81bd020 — "review: move shellcheck disable directives inside run: script bodies"

Changes: .github/workflows/marker-emission-aggregate.yml

This commit corrects the placement of shellcheck disable directives at three locations (lines 42, 127, 159).

The Fix ✅

Before (incorrect):

- name: Step name
  # shellcheck disable=SC2016
  run: |
    script content

After (correct):

- name: Step name
  run: |
    # shellcheck disable=SC2016
    script content

Why This Matters

The previous placement had shellcheck directives as YAML comments (outside the script body). Shellcheck processes the actual shell script content passed to run:, not the YAML structure around it — so those directives were invisible to shellcheck and had no effect.

Moving them inside the script body (first line after run: |) makes them proper bash comments that shellcheck will parse and honor. This is the correct pattern referenced in CLAUDE.md's convention for suppressing SC2016 on GitHub Actions template expressions.

Code Quality ✅

• Correct fix: Shellcheck directives are now positioned where the linter can actually see them
• Consistent: All three instances follow the same pattern
• Clean: No other changes to the workflow logic

Security ✅

• No security implications — purely a linting directive placement fix

Performance ✅

• No performance impact — workflow behavior unchanged

Test Coverage ✅

• No test changes needed — this is a non-functional lint configuration fix

Documentation ✅

• Commit message clearly states the intent
• Follows CLAUDE.md guidance on shellcheck directives

---

Recommendation: APPROVE ✅

This commit correctly fixes the shellcheck directive placement from the previous commit. The directives are now positioned where shellcheck can actually process them (inside the script body, not as YAML comments). Good catch and clean fix.

[github-actions]

Claude finished @cbeaulieu-gt's task in 1m 18s —— View job

---

PR Review Complete ✅

I've reviewed the latest incremental change (81bd020 → 21e87ca).

Todo List

• Fetch incremental diff (81bd020 → 21e87ca)
• Assess if foundational code is touched — No: additional shellcheck directive placements, reviewing incrementally
• Analyze the changes
• Evaluate code quality, security, performance, test coverage, documentation
• Post comprehensive review feedback

---

Summary

Commit reviewed: 21e87ca — "review: add per-statement shellcheck disable before each jq/printf block"

Changes: .github/workflows/marker-emission-aggregate.yml

This commit adds shellcheck disable directives at three additional locations (lines 51, 167, 178) — completing the lint cleanup started in the previous two commits.

The Pattern ✅

Each new directive follows the correct pattern established in commit 81bd020:

run: |
  # shellcheck disable=SC2016
  COMMAND=$(statement with ${{ }} or other SC2016 trigger)

Three additions:

1. Line 51 — Before gh api call with --jq and --arg:

   # shellcheck disable=SC2016
   MERGED_PRS=$(gh api "repos/$REPO/pulls?..." \
     --jq --arg since "$SINCE" \
     '[.[] | select(...)]')

   The jq expression contains $since (a jq variable) which shellcheck incorrectly flags as an unexpanded shell variable.

2. Line 167 — Before gh issue list call with --jq:

   # shellcheck disable=SC2016
   EXISTING=$(gh issue list \
     --repo "$REPO" \
     --state open \
     --json number,title \
     --jq ".[0].number // \"\"")

   The jq filter ".[0].number // \"\"" contains $ in the alternative operator context, triggering SC2016.

3. Line 178 — Before printf call with format string:

   # shellcheck disable=SC2016
   BODY_TEXT=$(printf \
     '**Week of:** %s\n\n**marker_missing count:** %s...' \
     "$WEEK_OF" "$MARKER_MISSING" ...)

   The format string is single-quoted and contains literal text that looks like markdown, which shellcheck may flag.

Why These Are Correct

SC2016 warns about $var inside single quotes where shell expansion won't happen. In all three cases:

• The single quotes are intentional (passing literal strings to jq or printf)
• Any $ characters are either jq variables (not shell variables) or literal text
• The directives correctly suppress false-positive warnings

This follows CLAUDE.md's documented convention for GitHub Actions workflows.

Code Quality ✅

• Correct placement: All directives are inside the run: script body (not YAML comments)
• Consistent: Same pattern as the previous two fix commits
• Complete: These appear to be the remaining SC2016 warnings in the file
• Clean: No logic changes, purely linting directives

Security ✅

• No security implications — lint configuration only
• The underlying shell commands are unchanged

Performance ✅

• No performance impact — workflow behavior is identical

Test Coverage ✅

• No test changes needed — non-functional change
• The workflow's logic and outputs are unchanged

Documentation ✅

• Commit message clearly describes the scope: "per-statement shellcheck disable before each jq/printf block"
• Inline comments follow the standard shellcheck directive format

---

Recommendation: APPROVE ✅

This commit completes the shellcheck lint cleanup for marker-emission-aggregate.yml. All three directives are correctly placed, appropriately scoped, and follow repository conventions. The file should now be actionlint-clean. No issues found.