Port pr-security-preflight review-thread resolution to the pack

[justin808]

Brings the portable pack's pr-security-preflight to parity with react_on_rails' copy (which had diverged ahead via #4148/#4151/#4170) — porting only the GENERIC security improvements and leaving RoR's hosted-CI metadata recognition as a RoR-local override.

Ported (generic, repo-agnostic)

• Review-thread resolution — a resolved trusted-bot review comment is treated as historical review metadata and suppressed; unresolved or untrusted-resolved comments still block; fail-closed if resolution state can't be fetched. (RESOLVED_REVIEW_THREADS_QUERY + resolved_review_threads_page / add_resolved_review_comment_ids / next_review_threads_cursor / resolved_review_comment_database_ids; rest_context populates the id set, suspicious_pr_review_comments consumes it.)
• Untrusted-interaction decomposition — the monolithic untrusted_interaction_findings split into per-surface helpers (issue comments / review comments / reviews).

Intentionally left out (RoR-specific)

The hosted_ci_* metadata recognition encodes RoR's labels (ready-for-hosted-ci), script path (script/ci-changes-detector), and comment vocabulary — textbook repo-specific seam values. Porting it would ship dead, trust-widening machinery into a portable pack (YAGNI; one consumer). RoR keeps it as a thin local override.

Verification

• bin/validate green; preflight suite 13 → 16 tests (resolved / unresolved / untrusted-resolver cases with new fake-gh fixtures).
• Every ported function is byte-faithful to RoR's source of truth.
• Hard grep confirms zero repo-specific strings entered the pack (hosted ci, the labels, the script path, github_actions_bot, …).
• scan_target keeps upstream's direct participant_findings call (no participant_findings_for_target, no hosted_ci_metadata_actors).

[coderabbitai]

Warning

Review limit reached

@justin808, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 20 minutes and 35 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4154d022-2e04-41d3-82f5-e507de086217

📥 Commits

Reviewing files that changed from the base of the PR and between 5072f45 and 28325b3.

📒 Files selected for processing (2)

• skills/pr-batch/bin/pr-security-preflight
• skills/pr-batch/bin/pr-security-preflight-test.rb

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/upstream-preflight-review-resolution

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28325b3a68

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Paginate resolved review thread comments

When a resolved review thread has more than 100 comments, this query only returns the first 100 databaseIds. The REST review-comment scan still inspects all pull comments, so a suspicious trusted-bot comment later in the same resolved thread is absent from resolved_review_comment_ids and incorrectly remains a blocker despite the thread being resolved by a trusted actor.

Useful? React with 👍 / 👎.

[claude]

Security Preflight Review — PR #5

This PR ports review-thread resolution suppression from react_on_rails to the portable pack: resolved trusted-bot review comments are now treated as historical review metadata and skipped during the suspicious-text scan. The porting is disciplined — the RoR-specific hosted-CI path was intentionally left out, the pagination loop is correct, the fail-closed fallback (rescue StandardError → Set.new) is correct, and the 3 new tests cover the resolved/unresolved/untrusted-resolver cases. Net quality is an improvement. A few things worth a second look:

---

1 · Resolved-bot comments are suppressed from warnings too — is that intended? (medium)

suspicious_pr_review_comments fires the next if trusted_bot? && resolved_ids.include? guard before the pattern test, so a resolved trusted-bot comment is excluded from both the blocking-findings call and the warning-findings call. The inline comment says "Keep unresolved/current bot findings blocking", leaving warnings ambiguous. If a bot leaves GITHUB_TOKEN in a review comment and a trusted user resolves the thread, that token mention vanishes from output entirely — no blocking finding, no warning. If that's the intent ("treat all resolved-bot content as historical"), the comment should say so explicitly; if warnings should still appear, the guard needs to be narrowed to blocking calls only.

See inline comment on skills/pr-batch/bin/pr-security-preflight:376.

---

2 · comments(first:100) — inner comment pagination is unhandled (low)

The RESOLVED_REVIEW_THREADS_QUERY fetches comments(first:100) per thread without a cursor or inner pagination loop. For a thread with more than 100 comment replies, the extra databaseIds are silently excluded from resolved_review_comment_ids. The consequence is fail-closed (the missed comments keep blocking), but a thread with 101+ comments is not as implausible as a thread with 10,000 nodes. A note in the code or a TODO is probably sufficient; adding a full inner pagination loop may not be worth the complexity.

See inline comment on skills/pr-batch/bin/pr-security-preflight:193.

---

3 · trust_coderabbit embeds a product name in the shared skill's test (low)

The new trust_coderabbit helper hard-wires coderabbitai as the trusted bot. Both the method name and the YAML content encode a specific consumer product choice inside a portable pack test. The two pre-existing tests (e.g. test_hidden_trusted_bot_participant_is_allowed) spell the YAML out inline, which is better — it keeps the bot name visible at the call site and avoids making the test a de-facto endorsement of one vendor. Consider renaming to something like write_trust_config(path, bots: ["coderabbitai"]) so the bot name is an explicit argument rather than baked into the implementation.

See inline comment on skills/pr-batch/bin/pr-security-preflight-test.rb:269.

---

4 · Three-mode condition repeated five times in the fake-gh script (minor cleanup)

[ "$mode" = "resolved-trusted-bot-review-comment" ] || [ "$mode" = "untrusted-..." ] || [ "$mode" = "unresolved-..." ] appears verbatim at five separate points in the generated bash script. Adding a fourth test mode in this group requires editing five heredoc sites with no mechanical safety net. A shell helper function hoisted to the top of the script (is_bot_review_comment_mode() { ...; }) would collapse each site to a readable one-liner.

---

5 · Stray blank line inside untrusted_issue_comment_findings (nit)

There is a blank line between filter_map do |comment| and untrusted_interaction_finding( that the two peer functions (untrusted_review_comment_findings, untrusted_review_findings) don't have. Minor inconsistency.

See inline comment on skills/pr-batch/bin/pr-security-preflight:460.

[claude]

This guard fires before the pattern test, so it suppresses the comment from both the blocking-findings pass and the warning-findings pass. A resolved trusted-bot comment containing GITHUB_TOKEN (warning-only) produces zero output — no blocking finding and no warning.

If the intent is to treat all content from resolved bot threads as historical metadata (including warning-level content), the inline comment should say so. If warnings should still surface for resolved threads, the guard needs to be conditional on the active pattern:

Suggested change

	next if trusted_bot?(author, trust_config) && resolved_review_comment_ids.include?(comment["id"])
	next if trusted_bot?(author, trust_config) && resolved_review_comment_ids.include?(comment["id"]) &&
	pattern == BLOCKING_SUSPICIOUS_PATTERN

Or add a # Both blocking and warning findings are suppressed for resolved trusted-bot comments. note to make the current intent unambiguous.

[claude]

comments(first:100) fetches at most 100 nodes per thread with no inner cursor or pagination loop. A thread with more than 100 comment replies will have some databaseIds silently excluded from resolved_review_comment_ids.

The consequence is fail-closed — missed IDs remain blocking — so this isn't a security bypass. But it is a silent truncation. Consider a # NOTE: threads with >100 comments are fail-closed — missing IDs stay blocking comment, or a hasNextPage check in add_resolved_review_comment_ids that warns rather than silently drops.

[claude]

This helper bakes coderabbitai into the shared pack's test, coupling a generic fixture to a specific vendor's product. The two pre-existing tests that need a trusted bot (e.g. test_hidden_trusted_bot_participant_is_allowed) spell out the YAML inline, which keeps the product name visible and local to the call site.

Consider a generic helper instead:

Suggested change

	def trust_coderabbit(trust_config_path)
	def write_trust_config(trust_config_path, bots: [], users: ["justin808"])
	File.write(trust_config_path, <<~YAML)
	trusted_users:
	#{users.map { |u| "- #{u}" }.join("\n ")}
	trusted_bots:
	#{bots.map { |b| "- #{b}" }.join("\n ")}
	trusted_teams: []
	YAML
	end

Call sites become write_trust_config(trust_config_path, bots: ["coderabbitai"]), keeping the product name explicit at each test rather than hidden inside the helper name.

[claude]

Spurious blank line after do |comment| — the two peer functions (untrusted_review_comment_findings, untrusted_review_findings) omit it. Remove for consistency.

Suggested change

	rest_context.fetch(:issue_comments).filter_map do |comment|

[claude]

This [ "$mode" = "resolved-..." ] || [ "$mode" = "untrusted-..." ] || [ "$mode" = "unresolved-..." ] condition appears verbatim at 5 points in the generated bash script (lines 339, 374, 426, 464, 472). Adding a fourth mode in this group requires editing all five heredoc sites with no safety net.

A shell helper hoisted to the top of the generated script would collapse each site:

is_bot_review_comment_mode() {
  [ "$mode" = "resolved-trusted-bot-review-comment" ] || \
  [ "$mode" = "untrusted-resolver-trusted-bot-review-comment" ] || \
  [ "$mode" = "unresolved-trusted-bot-review-comment" ]
}

Then each branch becomes if is_bot_review_comment_mode; then.