feat(pr-review): switch to App-token identity (#250)

[cbeaulieu-gt]

Closes #250

Summary

Reverses the deliberate github.token carve-out documented in #114 for the pr-review action. Under a single-operator personal-repo constraint, the cost of the carve-out (review comments posting as github-actions[bot] while every other Claude-powered workflow posts as the App's [bot] identity) outweighs the benefit (avoiding consumer onboarding regression, which doesn't bind here).

Changes

• pr-review/action.yml
  • Adds app_id and app_private_key composite-action inputs.
  • Inserts actions/create-github-app-token@1b10c78… # v3.1.1 (SHA-pinned, matching apply-fix/lint-failure/tag-claude pattern) and a Resolve review token step that emits a clear error if both inputs are missing.
  • claude-code-action@v1 step now consumes ${{ steps.token.outputs.value }} for github_token (was ${{ github.token }}).
  • Four bot-comment selectors updated atomically from literal github-actions[bot] match to endswith("[bot]"):
    • Line 299 — quality-gate authoritative selector
    • Line 370 — synthesis step (already had timestamp guard)
    • Line 480 — shadow-gate fallback selector
    • Line 589 — incomplete-review check (uses gh pr view --json comments — author.login not user.login)
  • Selector hardening (review feedback): the authoritative gate (line 299) and shadow-gate fallback (line 480) now also gate by \$REVIEW_START_TIME, mirroring the synthesis step. Without this, a foreign bot (Dependabot, Renovate, CodeQL) updating its comment after Claude's review finishes could silently win the sort under the widened endswith("[bot]") matcher.
• .github/workflows/claude-pr-review.yml — adds app_id/app_private_key to the workflow_call secrets: block (required: true), wires through to the composite step with secrets.app_id || secrets.APP_ID fallback.
• CLAUDE.md — ## Key conventions § Token selection updated to reflect that all workflows now use the App token; carve-out reversed under feat(pr-review): switch to App-token identity (revisit #114 carve-out) #250. Required-secrets table lists pr-review under APP_ID/APP_PRIVATE_KEY.
• README.md — Quick Start example, prerequisites, and migration breaking-changes section updated.

Test plan

• CI: actionlint passes (verified locally).
• Dogfood: this PR's own claude-pr-review job runs under the new App identity and posts a sticky comment that endswith("[bot]").
• Quality-gate status check posts on this PR using the new selector + timestamp filter; agrees with shadow gate.
• Floated v2 against an external consumer (siege-web or similar) — review run posts under App identity, no consumer regression.
• Verify no foreign-bot interference: this repo currently has no Dependabot/Renovate/CodeQL configured, but the REVIEW_START_TIME guard makes the selector resilient if any are added later.

Notes

• Breaking for consumers of glitchwerks/github-actions/.github/workflows/claude-pr-review.yml@v2: callers must now pass app_id and app_private_key secrets. Migration guidance is in the updated README.
• The fourth selector (line 589) uses gh pr view --json comments which exposes author.login rather than the REST API's user.login — both code paths updated.

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

[github-actions]

Claude finished @cbeaulieu-gt's task in 2m 59s —— View job

---

Claude PR Review — Complete

Reviewed PR #252: feat(pr-review): switch to App-token identity (#250)

This PR reverses the github.token carve-out for pr-review and switches to App-token identity for consistency across all Claude-powered workflows. The implementation follows established patterns from other actions and includes defensive hardening via REVIEW_START_TIME gating to prevent foreign bot interference.

---

Findings

🟢 Medium — Input validation mismatch creates confusing failure mode

pr-review/action.yml:9-13, .github/workflows/claude-pr-review.yml:15-18

The composite action declares app_id and app_private_key as required: false, but the reusable workflow marks these secrets as required: true. This creates a state where:

• External consumers calling the workflow must provide the secrets (enforced at workflow level)
• Internal calls to the composite action accept missing inputs but fail at runtime (line 222-223)

The runtime error message is clear, but this inconsistency is confusing:

# pr-review/action.yml
app_id:
  description: 'GitHub App ID for generating a short-lived token (required for App-identity comments)'
  required: false  # ← Should this be true?

Recommendation: Consider marking these inputs as required: true in the composite action to match the workflow contract and fail fast with a clear validation error. Alternatively, document why they're marked optional if there's a future path that doesn't require them.

Impact: Low — the error is caught at runtime with a clear message, but the mismatch creates unnecessary confusion during integration.

---

🟢 Medium — Timestamp comparison might pick up foreign bot in edge case

pr-review/action.yml:305, 376, 489

All three bot-comment selectors use >= "$REVIEW_START_TIME" to filter comments:

map(select((.user.login | endswith("[bot]")) and .updated_at >= "$REVIEW_START_TIME"))

Using >= creates a theoretical race: if Dependabot (or another bot) updates its comment with updated_at exactly equal to REVIEW_START_TIME, it would pass the filter and could be selected instead of Claude's comment.

Recommendation: Change the timestamp comparison from >= to > to ensure only comments updated after the review started are selected. This makes the filter strictly exclusive of any pre-existing bot activity.

-map(select((.user.login | endswith("[bot]")) and .updated_at >= "$REVIEW_START_TIME"))
+map(select((.user.login | endswith("[bot]")) and .updated_at > "$REVIEW_START_TIME"))

Impact: Very low probability in practice (requires exact timestamp collision to the second in ISO 8601), but the fix is trivial and eliminates the edge case entirely.

---

Nit — Error message assumes missing inputs rather than token generation failure

pr-review/action.yml:222-223

The "Resolve review token" step's error message assumes the inputs are missing:

echo "::error::No authentication token provided. Set app_id + app_private_key inputs. See README for GitHub App setup instructions."

However, the step also runs when app_id and app_private_key are provided but the App token generation fails (e.g., invalid private key format, expired credentials, network error). In that case, the error message is misleading.

Recommendation: Differentiate between missing inputs and token generation failure:

if [ -n "${{ inputs.app_id }}" ] && [ -n "${{ inputs.app_private_key }}" ]; then
  echo "::error::GitHub App token generation failed. Check APP_ID and APP_PRIVATE_KEY validity."
else
  echo "::error::No authentication token provided. Set app_id + app_private_key inputs. See README for GitHub App setup instructions."
fi
exit 1

This provides clearer diagnostics for troubleshooting.

Impact: Developer experience — clearer error messages reduce time to resolution when integration fails.

---

Nit — Selector synchronization relies on manual discipline

pr-review/action.yml:305, 376, 489 (and comments at lines 294, 370, 479)

Three separate code locations use nearly-identical jq selectors for bot comments:

1. Authoritative quality gate (line 305)
2. Synthesis step (line 376)
3. Shadow gate fallback (line 489)

The comments explicitly note these must stay synchronized (~line 294: "Mirrors the synthesis step at ~line 370 and the incomplete-review check at ~line 589"). If a future edit updates one selector but not the others, the gates could diverge and produce inconsistent results.

Recommendation: Extract the common jq filter expression into a shell variable or sourced function (similar to how SEVERITY_REGEX_SH is sourced) to eliminate duplication:

# In a new file lib/bot-comment-selector.sh:
BOT_COMMENT_JQ_FILTER='map(select((.user.login | endswith("[bot]")) and .updated_at > $review_start)) | sort_by(.updated_at) | last'

# In action.yml:
source "${{ github.action_path }}/lib/bot-comment-selector.sh"
BODY=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments?per_page=100" \
  --jq "$BOT_COMMENT_JQ_FILTER | .body // \"\"" \
  --arg review_start "$REVIEW_START_TIME")

This ensures all three callsites use the exact same logic.

Impact: Maintainability — prevents future drift between selectors that must agree.

---

Strengths

✅ Consistent identity across workflows — App-token usage aligns pr-review with tag-claude, lint-failure, ci-failure, and apply-fix. This eliminates the confusing mixed-identity state where most actions posted as my-app[bot] but pr-review posted as github-actions[bot].

✅ Defensive selector hardening — Adding REVIEW_START_TIME gating to all three bot-comment selectors (authoritative gate line 305, synthesis line 376, shadow gate line 489) prevents foreign bots (Dependabot, Renovate, CodeQL) from winning the sort_by(.updated_at) | last selection. Without this guard, a Renovate comment updated after Claude finishes could silently hijack the quality gate. This addresses the core risk identified in #250.

✅ SHA-pinned action dependency — actions/create-github-app-token@1b10c78... is digest-pinned following security best practices. Matches the pattern used in apply-fix, lint-failure, and tag-claude (consistency across the library).

✅ Clear breaking-change documentation — README.md and CLAUDE.md both updated to reflect the new requirement. Migration steps are explicit, and the "Required secrets" table correctly lists pr-review under APP_ID / APP_PRIVATE_KEY.

✅ Dogfooding validation — This PR's own claude-pr-review job runs under the new App identity, providing immediate smoke-test coverage for the changes.

---

Code Quality

The implementation is clean and follows established patterns. Variable naming is clear (APP_TOKEN, REVIEW_START_TIME), and inline comments explain the reasoning well (e.g., the detailed comment at lines 287-295 explaining why REVIEW_START_TIME gating is necessary). The refactoring from literal github-actions[bot] to endswith("[bot]") is applied atomically across all four selectors.

---

Security

No security regressions identified. The switch to App tokens is a lateral move (both github.token and App tokens are short-lived and scoped). SHA-pinning of the create-github-app-token action prevents supply-chain attacks. The app_private_key is passed through GitHub Actions secrets, which is the correct mechanism.

One minor note: there's no validation that the app_private_key is well-formed PEM before attempting token generation, but this is consistent with how other actions (apply-fix, lint-failure, tag-claude) handle it — the create-github-app-token action itself will reject invalid keys.

---

Performance

No performance impact. The API calls remain the same; only the filtering logic changes. The endswith("[bot]") jq predicate is efficient (string suffix check), and the timestamp comparison adds negligible overhead.

---

Test Coverage

This repository has no automated test suite (per CLAUDE.md: "There are no other build steps, tests, or package managers"). The test plan in the PR description is manual but comprehensive:

• ✅ actionlint passes
• ✅ Dogfood: this PR's own review job exercises the new code path
• ✅ Quality-gate status check posts using new selector + timestamp filter
• ✅ External consumer validation planned (siege-web float)

The lack of automated tests means the selector synchronization issue (Nit finding above) relies entirely on code review discipline to catch drift.

---

Documentation

Documentation is thorough and accurately reflects the changes:

• CLAUDE.md — "Token selection for claude-code-action" section updated to explain that all workflows now use App token (line 63). The carve-out rationale is preserved for historical context but marked as reversed under feat(pr-review): switch to App-token identity (revisit #114 carve-out) #250.
• README.md — Prerequisites section updated (line 5), breaking changes section expanded (lines 162, 165), and Quick Start example updated (lines 64-65).
• Inline comments — The new comments at lines 287-295 (authoritative gate), 370 (synthesis), and 474-479 (shadow gate fallback) explain the REVIEW_START_TIME gating rationale and cross-reference the other callsites that must stay in sync.

The migration guidance could be slightly more explicit about the failure mode (i.e., "If you don't update, your workflow will fail at the 'Resolve review token' step with an explicit error message"), but the current documentation is sufficient.

---

Verdict: APPROVE

[cbeaulieu-gt]

v2.6.0 release withdrawn. This PR was shipped as part of v2.6.0, but the release was rolled back: v2 floating tag is now back at e92a334 (v2.5.1).

Reason: the App-token requirement introduced here is breaking for any consumer pinned to an older v2.x.x reusable workflow, because those older wrappers do not pass app_id / app_private_key through. The combination with #254's API-fallback bug (#255) made the regression visible on the first real consumer run (glitchwerks/mom-bot#21).

Tracking the rollout-process problem under #256. The code change here is correct on its own; the failure was in the release/distribution path. A successor release will ship once the consumer-impact gate is in place.

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