[codex] Harden preflight review trust checks

[justin808]

Summary

• Port the React on Rails PR #4288 review hardening into the shared pr-security-preflight helper.
• Treat explicit trust configs outside the consumer repo as global, fail closed when AGENT_WORKFLOWS_TRUST_CONFIG points at a missing file, and warn on unqualified team slugs in global configs.
• Keep metadata-only bot text out of actionable trust while still warning-scanning it, and require complete source actor coverage before downgrading suspicious diff text.
• Add regression coverage and an Unreleased changelog entry.

Validation

• ruby skills/pr-batch/bin/pr-security-preflight-test.rb
• git diff --check
• bin/validate

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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 reviews.

How do review 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 refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f871ea15-98a4-4aa2-81e8-f1861b006c22

📥 Commits

Reviewing files that changed from the base of the PR and between ac08178 and 4f60219.

📒 Files selected for processing (7)

• CHANGELOG.md
• README.md
• docs/security-posture.md
• docs/trust-and-preflight.md
• skills/pr-batch/bin/pr-security-preflight
• skills/pr-batch/bin/pr-security-preflight-test.rb
• skills/pr-batch/lib/git_probe_env.rb

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg-codex/preflight-review-hardening

---

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.}

[claude]

Review: Harden preflight review trust checks

This PR ports four trust-resolution hardening fixes into the shared pr-security-preflight helper: fail-closed on a missing AGENT_WORKFLOWS_TRUST_CONFIG file, treat explicit out-of-repo configs as global, warn on unqualified team slugs in global configs, and extend suspicious-text warning scans to metadata-bot comments. The regression tests are comprehensive. Three issues found:

1. canonical_path rescues StandardError instead of Errno::ENOENT — security-relevant misclassification possible

File.realpath can also raise Errno::EACCES, Errno::ELOOP, and Errno::ENOTDIR. When one of those fires for one path but not the other inside path_inside_git_root?, one return value is a fully-resolved realpath and the other is an unexpanded logical path; the start_with? comparison becomes unreliable. Common trigger: a symlinked home directory where git toplevel returns /home/user/repo (symlink to /data/users/user/repo) and a permission error on the config path falls back to the un-resolved form — path_inside_git_root? returns false for an in-repo config, setting global: true and widening trust scope incorrectly. Fix: narrow the rescue to Errno::ENOENT.

2. suspicious_pr_reviews not updated — metadata-bot full review bodies are never warning-scanned

suspicious_pr_review_comments was updated to use actor_trusted_for_suspicious_warning? (which includes trusted_metadata_bot?), but the sibling suspicious_pr_reviews still guards on trusted_actor?. Bots like CodeRabbit file full PR reviews (not just review comments); their review bodies containing GITHUB_TOKEN, curl, eval, etc. are silently skipped rather than surfaced as warnings.

3. actor_trusted_for_suspicious_warning? is an exact duplicate of allowed_metadata_actor?

The new helper (lines 525-528) has the same signature and identical body as the existing allowed_metadata_actor? at line 332. The call sites in suspicious_issue_text and suspicious_pr_review_comments could call allowed_metadata_actor? directly; the new function should be removed.

[claude]

Review: Harden preflight review trust checks

This PR ports security hardening from a consumer repo into the shared pr-security-preflight helper. The core changes are directionally correct and well-covered by the new test cases.

Three bugs survived verification:

Bug 1 - Issue body not scanned for trusted_metadata_bot authors (medium)

suspicious_issue_text updated the issue comments loop to allowed_metadata_actor? but left the issue body check (a few lines above, also in suspicious_issue_text) still gated by trusted_actor?. A bot listed only in trusted_metadata_bots that authored the issue/PR has its body completely unscanned for injection patterns - the exact blind spot the PR set out to close. Fix: change trusted_actor? on the issue body guard to allowed_metadata_actor?.

Bug 2 - Resolved trusted_metadata_bot review comments are never suppressed (medium)

Inline comment posted on lines 559-560.

Bug 3 - canonical_path swallows only ENOENT, not ENOTDIR (low)

Inline comment posted on line 238.

[claude]

Security Preflight Hardening Review

This PR correctly tightens several trust-boundary gaps: explicit --trust-config paths outside the git root are now classified as global (enforcing OWNER/slug team format), a set env var pointing at a missing file aborts instead of silently falling through, timeline-item truncation now gates trusted_pr_source? alongside commit-author truncation, and resolved known-bot comments with blocking-pattern text surface as warnings rather than being silently suppressed. The new tests cover each new behaviour path. Two low-severity issues noted as inline comments.

Findings (low severity):

1. Dead mode conditions in elif at test line 1271 — metadata-bot-review and resolved-metadata-bot-warning-review-comment are added to the elif branch for repos/owner/repo/issues/123, but they are already caught by the earlier if at line 1247 (same PR-payload JSON, so no wrong output today). The conditions in the elif are unreachable and will silently mislead anyone who reorganises this block.

2. Unchecked git init return value in new test — system("git", "-C", consumer_root, "init", "--quiet") in test_explicit_repo_local_trust_config_allows_unqualified_team_slugs ignores the return value. If git init fails (missing binary, safe-directory restriction, or nesting forbidden by safe.directory), path_inside_git_root? sees no git root, classifies the config as global, drops the bare team slug, and the test fails with SECURITY_PREFLIGHT_BLOCKED instead of a clear setup-error message.

[claude]

Review: Harden preflight review trust checks

This PR correctly tightens trust resolution on three fronts: explicit configs outside the git root are now classified global (triggering team-slug warnings), ENV trust configs pointing at missing files now abort rather than silently falling through to the home config, and the diff-safety downgrade in trusted_pr_source? now also requires complete timeline coverage — not just complete commit-author coverage. The new tests are well-structured and cover the new code paths. Two issues worth addressing:

1. Object-identity check for BLOCKING_SUSPICIOUS_PATTERN is fragile

skills/pr-batch/bin/pr-security-preflight line 563:

next if resolved_comment_from_known_bot && !pattern.equal?(BLOCKING_SUSPICIOUS_PATTERN)

equal? tests Ruby object identity, not Regexp equality. Today every caller passes the constant directly, so it works. But if any future refactor passes Regexp.new(BLOCKING_SUSPICIOUS_PATTERN.source) (or a cached copy), equal? returns false, the next fires for both patterns, and blocking-injection text in resolved trusted-bot comments is silently skipped — the exact scenario this hunk is designed to prevent.

Use pattern == BLOCKING_SUSPICIOUS_PATTERN instead; Regexp == compares source and options.

2. New repo-local test inherits GIT_DIR from the parent process

skills/pr-batch/bin/pr-security-preflight-test.rb test_explicit_repo_local_trust_config_allows_unqualified_team_slugs passes bare env (only PATH and PREFLIGHT_TEST_MODE) to run_script. The subprocess therefore inherits GIT_DIR / GIT_WORK_TREE from whatever launched the test suite. If either is set (git hooks, IDE Git integrations, some CI environments), git_toplevel inside the script returns the host repo root instead of the freshly-init'd consumer_root, path_inside_git_root? classifies the config as global, the unqualified-team-slug WARN fires, and the refute_includes out, "WARN: …" assertion fails.

Fix: unset those vars in the env merge, e.g.:

out, status = run_script(
  env.merge("GIT_DIR" => nil, "GIT_WORK_TREE" => nil),
  "--repo", "owner/repo",
  "--trust-config", repo_config,
  "123",
  chdir: consumer_root
)

In Ruby's spawn/Open3, setting an env key to nil explicitly unsets it for the child.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 917a2b882b

ℹ️ 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: efd3957ce8

ℹ️ 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3691802ad0

ℹ️ 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".

[claude]

Review: Harden preflight trust checks

This PR extends the trust-resolution pipeline with four well-scoped changes: fail-closed on a missing AGENT_WORKFLOWS_TRUST_CONFIG, classify explicit configs outside the consumer repo as global, warn on unqualified team slugs in global configs, and scan metadata-bot text for suspicious patterns while keeping it out of the actionable-trust gate. The regression test suite is thorough and covers every new code path.

Three findings survived verification:

CONFIRMED — Issue body not scanned for metadata-bot authors (line 533)

suspicious_issue_text was updated so issue comments use allowed_metadata_actor?, but the issue/PR body check at line 533 still uses trusted_actor?. A PR opened by a trusted_metadata_bot (e.g., an AI code-review bot that creates draft PRs) has its body invisible to the suspicious-text scanner, while that same bot's comments are fully scanned. The asymmetry is an incomplete migration and is the most concrete bug introduced by this PR.

PLAUSIBLE — suspicious_pr_reviews has no resolved-comment escape for warning patterns

suspicious_pr_review_comments gained a skip for resolved known-bot comments on non-blocking patterns (line 566), but suspicious_pr_reviews (PR review bodies, not inline comments) has no equivalent. A metadata bot that permanently posts review bodies containing warning-pattern text (e.g., GITHUB_TOKEN in a status summary) will generate a persistent warning with no operational path to clear it other than a PR-level acknowledgement override.

PLAUSIBLE — source_actor_coverage_findings omits participants connection vs. graph_coverage_findings

graph_coverage_findings covers all of PAGINATED_CONNECTIONS (%w[participants timelineItems]) plus commit authors. source_actor_coverage_findings hard-codes only timelineItems + commit authors. Not exploitable today because pr_source_actor_logins never reads from participants, but the structural inconsistency creates a silent gap if participants-based trust logic is added later without updating this guard.

[claude]

Review summary

Reviewed the trust-resolution and metadata-bot hardening in pr-security-preflight, the new skills/pr-batch/lib/git_probe_env.rb helper, and the doc/changelog updates.

Portability (AGENTS.md): No consumer-repo-specific commands, labels, branches, or paths leaked into skills/. The justin808 references in the test fixtures are pre-existing (already used elsewhere in this repo's tests/changelog as the example trusted user), not newly introduced consumer-specific data.

Correctness: Traced the git-remote-based repo/host matching (git_root_matches_repo?, github_remote_from_remote_url, remote_url_host) against the new regression tests (enterprise hosts, ports, ssh-over-https, scp-like syntax, malformed ports, newline-injection attempts, GIT_CONFIG_* / GIT_DIR / GIT_WORK_TREE env poisoning). The logic is consistent and each edge case has a corresponding test. The fail-closed behavior for a missing $AGENT_WORKFLOWS_TRUST_CONFIG path, and the expanded "global trust config" classification (only affects whether unqualified team slugs are accepted — team lookups are always scoped to the resolved repo's owner regardless), are sound; a misclassification here can't cross repo-owner boundaries.

Metadata-bot text scanning: The new trusted_metadata_text? / source_actor_coverage_findings changes correctly extend suspicious-text warning-scanning to metadata-only bots without making their content actionable-trust, and correctly require full timeline (not just commit-author) coverage before downgrading diff warnings for a "trusted" PR source. Confirmed the existing (unmodified by this PR) design where blocking-pattern text in comments/issue bodies/reviews always surfaces as a warning rather than a hard block — this is pre-existing, intentional, and documented behavior (docs/trust-and-preflight.md), not something this PR changes.

Shell/Ruby safety: git_probe_env.rb and the capture_git_probe wrapper appropriately clear GIT_* environment variables (including dynamically-named GIT_CONFIG_KEY_N/VALUE_N pairs) before shelling out to git, and restrict remote-URL discovery to --local/--worktree config scope so a poisoned ~/.gitconfig or global git config can't spoof a repo match. Open3.capture3/Timeout.timeout usage throughout avoids shell interpolation (argv arrays, not string commands), so no shell-injection surface.

No blocking issues found. Left two minor/non-blocking inline notes:

1. Two separately-named, undocumented timeout env vars (PR_SECURITY_PREFLIGHT_GIT_TIMEOUT_SECONDS vs PR_BATCH_GIT_PROBE_TIMEOUT_SECONDS) control conceptually the same git-probe timeout.
2. repo_option_info now always makes one extra gh repo view call (unless GH_HOST is set), even when --repo is passed explicitly — a small new network dependency on a previously network-free codepath, worth a one-line doc/changelog mention if intentional.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 13877472bb

ℹ️ 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".

[claude]

Review: Harden preflight review trust checks

This PR extends pr-security-preflight with GitHub-host-aware trust-config classification (repo-local vs. global), fail-closed handling for a missing $AGENT_WORKFLOWS_TRUST_CONFIG file, warnings for unqualified team slugs in global configs, and a trusted_metadata_text? helper consolidating several suspicious-text call sites, plus a new shared PrBatchGitProbeEnv module for sanitizing git-related env vars before shelling out. The regression-test coverage is extensive.

I traced the new git-remote-URL parsing/host-matching logic (the mechanism that decides whether a trust config's git working tree matches the target repo) and found one real correctness bug, plus a design observation. Details below as inline comments.

Summary of findings:

1. remote_matches_github_host? (skills/pr-batch/bin/pr-security-preflight ~L391-409) has inconsistent/incomplete default-port handling: scp-style SSH remotes (git@host:owner/repo) never carry a :scheme, so they always fail to match whenever GH_HOST includes an explicit port; and even proper ssh:// remotes at the standard port 22 are compared against a hardcoded github_port == 443, never 22. This silently downgrades legitimate repo-local trust configs to "global" for common enterprise SSH remote setups — none of the new tests exercise GH_HOST with an explicit port combined with an scp-style or default-port ssh:// remote, so the gap isn't caught by CI.
2. (Design note) The ~150 lines of new hand-rolled remote-URL parsing/host-comparison logic duplicate a purpose the file already delegates to gh elsewhere (current_repo_info/repo_option_info call gh repo view --json url). The bug above is a direct symptom of reimplementing GitHub's remote-URL/port conventions instead of resolving the trust-config's repo via gh repo view -C <root> and reusing the existing repo_info_from_payload comparison path.

No security-severity or portability issues found — the new logic is appropriately scoped to skills/pr-batch/, and I didn't find any consumer-repo-specific hardcoding.

[claude]

Review summary

This PR ports the React on Rails PR #4288 hardening work into pr-security-preflight: it makes explicit trust configs outside the consumer repo classify as global (fail-closed on unqualified team slugs), makes a missing $AGENT_WORKFLOWS_TRUST_CONFIG path abort instead of silently falling through, keeps metadata-only bot text warning-scanned instead of ignored, requires complete source-actor coverage before trusting a PR's source, and adds a new host/port-aware git-remote matcher (skills/pr-batch/lib/git_probe_env.rb + a large block of new functions in the main script) to classify trust configs as repo-local vs. global across GHES hosts/ports/schemes. It's backed by a substantial expansion of the test suite.

I ran 8 independent review passes (correctness, removed-behavior, cross-file, reuse/simplification/efficiency/altitude/conventions) plus targeted verification. Most candidate issues turned out to be non-issues — the security-relevant refactors (metadata-bot warning-scanning, resolved-bot-comment suppression, source_actor_coverage_findings, fail-closed trust-config resolution) all checked out as intended hardening with test coverage, not regressions. Portability/AGENTS.md conventions are clean — nothing hardcoded from the source consumer repo.

Two things worth fixing, posted as inline comments:

1. remote_url_host (skills/pr-batch/bin/pr-security-preflight:347-359) — the default-port case statement has branches for https/ssh but not http, so a plain http://host/owner/repo remote on the default port 80 always gets a spurious :80 suffix. That mismatches a bare (portless) GH_HOST, causing a genuinely repo-local trust config to be misclassified as global (fails closed — silently drops unqualified team-slug entries rather than opening anything up — but it's a real bug, and the new tests only cover the http case with an explicit :80 in GH_HOST, not the bare-hostname case that actually triggers it).
2. repo_info_from_payload (skills/pr-batch/bin/pr-security-preflight:173-175) — the fallback_repo: keyword parameter is dead code; no call site ever passes it non-nil, and the one caller that might care overwrites the returned repo via .merge(repo:) anyway.

Nothing else survived verification at this effort level.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a4ceba38c

ℹ️ 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd57c8f564

ℹ️ 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".

[claude]

Review summary

Focused review of the trust-resolution hardening in pr-security-preflight / lib/git_probe_env.rb.

Portability — no consumer-repo-specific commands, labels, branches, or paths were introduced in skills/ or docs/; git_probe_env.rb sits correctly under skills/pr-batch/lib/ per AGENTS.md.

Security / correctness — traced the new host/port-matching logic (remote_matches_github_host?, github_remote_from_remote_url, git_root_matches_repo?) and the git-remote-URL injection defenses against the new regression tests:

• The NUL-delimited git config --get-regexp parsing plus \A...\z-anchored regexes correctly reject embedded-newline injection attempts (verified against test_git_remote_url_newline_cannot_inject_matching_remote), since \z (not \Z) is used and URI.parse/the SSH-shorthand regex both reject raw newlines.
• --local/--worktree git config scopes are used (never --global/--system), and GIT_CONFIG_COUNT/GIT_CONFIG_KEY_*/GIT_CONFIG_VALUE_* injection via env is explicitly cleared in PrBatchGitProbeEnv.probe_env.
• The new fail-closed behavior for a missing $AGENT_WORKFLOWS_TRUST_CONFIG path, and treating an explicit --trust-config outside the target repo as "global" (enforcing OWNER/team-slug), are real hardening improvements — previously a mismatched/stale explicit trust config could silently authorize an unqualified team slug against the wrong org.
• Resolved-bot-comment handling now still warning-scans blocking-pattern text in resolved bot comments instead of silently dropping it, while still not blocking on it — consistent with the "text findings from comments/reviews are warnings only, diff lines are blocking" model already in place.

Shell/Ruby safety: all subprocess calls use Open3.capture3/Open3.capture2e with argument arrays (no shell-string interpolation of untrusted data); the test double's bash heredocs only interpolate test-controlled fixture strings.

Left one minor inline nit about having two separately-configurable git-probe timeouts. No blocking issues found from static/manual review.

Caveat: I was not able to execute ruby skills/pr-batch/bin/pr-security-preflight-test.rb or bin/validate in this sandboxed review environment (command execution was restricted), so this review is based on careful static tracing of the diff against the new test expectations rather than an actual test run. Recommend confirming CI is green before merge.

[claude]

Review: Harden preflight review trust checks (#45)

This PR ports a set of security-hardening fixes into pr-security-preflight: classifying trust configs as "global" vs "repo-local" by matching git remote hosts (with GitHub Enterprise host/port support), failing closed when $AGENT_WORKFLOWS_TRUST_CONFIG points at a missing file, gating metadata-bot text through a stricter helper, requiring complete source-actor coverage before downgrading suspicious diff warnings, and scrubbing inherited GIT_* env vars before shelling out to git (new skills/pr-batch/lib/git_probe_env.rb). The change is large (~1.4k lines, mostly tests) but the core logic is well-covered by an extensive Minitest suite, and the new behavior matches the updated docs/CHANGELOG.

Findings, most severe first (also left as inline comments where the diff allowed it):

1. skills/pr-batch/bin/pr-security-preflight:134 (run_gh) — unscrubbed subprocess environment for gh. The new capture_git_probe (line 222) explicitly clears inherited GIT_DIR/GIT_WORK_TREE/GIT_CONFIG_* before shelling out to git, with a dedicated test for that exact scenario (test_run_script_clears_inherited_git_environment). But run_gh's Open3.capture3("gh", *args) still runs with the ambient environment. current_repo_info calls gh repo view with no repo argument (used whenever --repo is omitted), and gh does its own git-based repo auto-detection internally — so the same "inherited git environment" scenario this PR closes for direct git calls could still make gh repo view resolve the wrong repo/host, which seeds the entire rest of the run's trust decisions. Other run_gh call sites always pass an explicit repo and aren't exposed. Worth applying the same scrubbing to the gh subprocess (at least for current_repo_info), or noting why it's out of scope.

2. skills/pr-batch/bin/pr-security-preflight:253 — packaged trust-config fallback still hardcodes global: false. Every other fallback in resolved_trust_config reworked by this PR (env var, home dir) is now global: true, but the final { path: PACKAGED_TRUST_CONFIG, global: false } branch wasn't touched. PACKAGED_TRUST_CONFIG ships with the tool, outside the consumer repo's git tree — it fits "global" at least as well as the env/home fallbacks. Left as global: false, it's the one remaining fallback that accepts unqualified team slugs without the OWNER/team-slug requirement this PR adds everywhere else. Pre-existing, not a regression, but since this PR reworked every other branch in the same function, it looks like an oversight worth closing in the same pass.

3. skills/pr-batch/bin/pr-security-preflight:390 (host_port) — greedy regex can misparse multi-colon hosts. /\A(.+):(\d+)\z/ against something like "2001:db8::8443" (an unbracketed IPv6-literal-like GH_HOST value) mis-splits into host="2001:db8:", port=8443 instead of rejecting it. Low real-world reachability — GH_HOST is normally a plain hostname, and this file's own URL parsing (URI.parse, which requires bracketed IPv6 authority) never produces such a string — but worth a guard if you want to close the edge case outright.

4. Minor efficiency: inferred_github_host_from_local_git and git_root_matches_repo?/trust_config_repo_local? each independently probe the same git root (git_toplevel + git_remote_urls) with no shared cache. On the failure-fallback path (--repo passed, GH_HOST unset, gh repo view <repo> fails), this spawns several redundant git subprocesses for what's usually the same working tree in one run. Not a correctness issue, just a small memoization opportunity.

5. Altitude (design note, non-blocking): the host-comparison logic (remote_url_host, normalized_remote_host, host_port, remote_matches_github_host?) is a set of layered special cases (the ssh.github.com[:443] alias, per-scheme default ports, [22, 443] treated as equivalent ssh ports) rather than one canonical-identity mechanism. A GHES instance with yet another port/scheme convention would need another hardcoded exception. Normalizing every remote and GH_HOST to a single canonical (host, port) tuple up front (folding aliases in at parse time) and then comparing tuples directly would generalize more cleanly, though this is a style/maintainability observation, not a bug.

No portability issues found — no consumer-repo-specific commands/labels/branches/paths were hardcoded in the touched skill files, and the new skills/pr-batch/lib/git_probe_env.rb helper stays inside the pr-batch skill folder that uses it, consistent with AGENTS.md's editing rules.

[claude]

Review summary

This PR hardens pr-security-preflight's trust resolution: it classifies explicit/global trust configs by matching git remotes against a resolved GitHub host (supporting GHE with non-default ports/SSH-over-HTTPS), fails closed when $AGENT_WORKFLOWS_TRUST_CONFIG points at a missing file, warns on unqualified team slugs in global configs, extends suspicious-text warning scans to metadata-only bots, and adds a large regression test suite plus a shared git_probe_env helper that scrubs inherited git env vars from probe subprocesses. The host/port matching logic and repo-local-vs-global classification are extensively covered by the new tests and held up under adversarial tracing.

I ran 8 parallel review passes (line-by-line scan, removed-behavior audit, cross-file call tracing, reuse/simplification/efficiency, altitude, and CLAUDE.md/AGENTS.md conventions). Findings, most severe first:

1. Scope creep in the new coverage check (skills/pr-batch/bin/pr-security-preflight — inline comment posted). target_author_coverage_finding is folded into the general-purpose graph_coverage_findings, which is computed for every scan target, not just PRs. Combined with record_blockers's unconditional (non-strict_trust-gated) github-api-coverage block, this means an ordinary issue authored by a deleted/suspended GitHub account (GraphQL author: null) will now hard-block every preflight run by default — a behavior that didn't exist before this PR for issues, and isn't exercised by any test (all issue/PR fixtures use non-empty authors except one PR-only fixture). The PR's own goal was narrower (gate trusted_pr_source?'s diff-warning downgrade on complete source-actor coverage); recommend scoping the author check to PR targets only, or keeping it exclusive to source_actor_coverage_findings.

2. Minor duplication (inline comment posted): source_actor_coverage_findings re-derives most of what graph_coverage_findings already computes (target author + commit author coverage), just swapping in a single timelineItems check instead of iterating PAGINATED_CONNECTIONS. Consolidating would avoid the two lists drifting apart later.

3. Minor efficiency nit (not blocking): git_toplevel and git_remote_urls can each be invoked 2–3 times for the same directory in a single run (repo_trust_config_path, trust_config_repo_local?, inferred_github_host_from_local_git), each git_remote_urls call spawning two subprocesses (--local + --worktree). This is bounded (not inside the per-target ARGV.each loop), so it's a one-time startup cost rather than something that scales with target count — flagging only as a cheap opportunity to memoize by root if this file gets touched again.

Portability / shared-skill hygiene: no consumer-repo-specific commands, labels, branches, or paths were found hardcoded in the shared skill code — all new seams ($AGENT_WORKFLOWS_TRUST_CONFIG, GH_HOST, PR_SECURITY_PREFLIGHT_GIT_TIMEOUT_SECONDS, PR_BATCH_GIT_PROBE_TIMEOUT_SECONDS) are generic and env-driven. The new skills/pr-batch/lib/git_probe_env.rb correctly stays inside the pr-batch skill folder per AGENTS.md's Editing Rules. No CLAUDE.md/AGENTS.md rule violations found.

Shell/Ruby helper safety: all git/gh subprocess calls consistently use array-form Open3.capture3/system (no shell string interpolation), so no injection risk from remote URLs, repo names, or trust-config paths — including the newline-injection edge case in git config --null --get-regexp output, which is explicitly tested (test_git_remote_url_newline_cannot_inject_matching_remote).

Nothing else survived adversarial verification at this effort level — the host/port default-port matching, GHE remote parsing, and metadata-bot warning-scan widening all checked out correctly against their test coverage.

[claude]

Review: Harden pr-security-preflight trust checks

Went through the full diff (script, new lib/git_probe_env.rb, the ~900 new test lines, and the doc/CHANGELOG updates) with a focus on correctness, security, and portability.

Portability - clean. Nothing in skills/ hardcodes a consumer-repo command, label, branch, or path; docs/CHANGELOG stay generic (the justin808 logins in test fixtures are pre-existing placeholder data, not new policy).

Shell/Ruby safety - solid. All subprocess calls use array-form Open3.capture3/system (no shell interpolation), git/gh calls are timeout-wrapped, and the new PrBatchGitProbeEnv.probe_env deliberately strips GIT_DIR/GIT_WORK_TREE/GIT_CONFIG_* (including dynamically-named GIT_CONFIG_KEY_N/VALUE_N) so an inherited or injected git environment can't redirect where git/gh think the repo lives. That's exercised by dedicated tests (test_script_git_probes_clear_injected_git_config_remotes, test_git_remote_url_newline_cannot_inject_matching_remote, test_malformed_remote_port_does_not_crash_preflight), which is the right level of paranoia for something feeding a trust decision.

Logic review - traced through the new host-inference (repo_option_info/current_repo_info/inferred_github_host_from_local_git) and repo-local-vs-global trust classification (trust_config_repo_local?, git_root_matches_repo?, remote_matches_github_host?, github_remote_from_remote_url) paths, including port/scheme edge cases (default ports, ssh.github.com:443, GHES with custom ports). Every failure mode I could construct (missing git, unmatched remotes, mismatched host/port, unparseable URLs) fails closed to 'global,' correctly forcing OWNER/team-slug qualification rather than silently trusting an unqualified slug. The suspicious-vs-suspicious_warnings split (only new diff lines can block; comment/review/issue text is warning-only regardless of pattern) is unchanged in spirit, and the new source_actor_coverage_findings correctly closes a real gap: trusted_pr_source? now also requires non-truncated timelineItems/target-author coverage before downgrading a diff warning, not just commit-author coverage.

No bugs found. Strong test coverage for a change this security-sensitive - no inline comments to add.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d389ae8623

ℹ️ 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".