feat(ci): exempt trusted bots from the independent-review-pending gate - #1859
Merged
Merged
Conversation
Design doc elicited via eliciting-a-design dialogue, approved by the user this session. Formalized into issue #1858 with its own Acceptance Criteria Map. Addresses 6 open Dependabot PRs blocked by the independent-review-pending required check, which has no path for a bot PR to record a human verdict. Refs #1858 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qxcd2iHFzURaCjfH9BXvVr
…ge-blocker-b3wwl6
Task Decomposition per executing-a-branch-plan step 3: 3 tasks (A: trusted-bots.yml allowlist + CODEOWNERS, B: gate script bot-exemption branch + tests, C: workflow wiring), no file-ownership conflicts, C sequenced after B on an interface-dependency edge. Refs #1858 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qxcd2iHFzURaCjfH9BXvVr
An independent pre-PR design-doc-adversarial-review (executing-a-branch- plan) found bot-identity matching against the PR opener alone (not the head commit) would let a non-bot commit pushed to a bot-opened PR's branch silently bypass independent review, since GitHub does not update the PR's user field on synchronize and dependabot/* branches are not covered by branch protection. Fix: require the head commit's own author/committer email to also match main.json's existing commit_author_email_pattern/committer_email_pattern rules for that bot; a mismatch falls through to the existing human-verdict path. Refs #1858 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qxcd2iHFzURaCjfH9BXvVr
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1859 +/- ##
==========================================
- Coverage 99.59% 99.56% -0.04%
==========================================
Files 168 168
Lines 27715 27966 +251
Branches 3342 3390 +48
==========================================
+ Hits 27602 27843 +241
- Misses 113 123 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Task A of issue #1858's Branch Plan: add .github/trusted-bots.yml (login/id/type/purpose per trusted bot, seeded with dependabot[bot]'s already-verified id 49699333 from .github/rulesets/main.json's commit_author_email_pattern/committer_email_pattern rules, PR #1843), and add CODEOWNERS coverage for it so an unreviewed edit can't silently widen who bypasses independent review. Also fixes tests/test_gitapex_sync_opencode.py::test_unreadable_skill_md_skipped, found failing pre-existing (reproduced identically with this task's own changes stashed out) because this repository's container runs the test suite as uid 0, where chmod(0o000) mode bits are bypassed and the read simply succeeds. Switched to a monkeypatched read_text raising PermissionError, mirroring the same fix already applied in test_gitapex_gate_commit_citation.py's test_main_commit_msg_an_unreadable_file_exits_two -- required to get the full verification suite green before this task could complete. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extend gitapex_gate_independent_review_pending.py with a bot-path branch, tried before the existing parse_verdict/check() human-verdict path and falling through to it whenever the bot-path conditions are not fully met (never modifying that existing path itself): - is_trusted_bot: matches a PR author's (login, id, type) against .github/trusted-bots.yml on all three fields together -- login alone is never sufficient. - head_commit_identity_matches_bot: the critical-defect fix from this issue's own design-doc Revision section. A bot-identity match on the PR's opener is not itself sufficient -- the head commit's own author/committer email must also match main.json's own commit_author_email_pattern/committer_email_pattern rules (PR #1843), the single source of truth for trusted-committer emails. A mismatch falls through to the human-verdict path rather than failing outright. - required_check_contexts: reads main.json's required_status_checks rule (mirroring, not importing, gitapex_gate_ruleset_required_checks.py's own rule_of_type() pattern), excluding this check's own name. Never misreads deletion/pull_request/commit_author_email_pattern/ committer_email_pattern rules as check contexts. Fails closed (an empty context list is never treated as "nothing to check"). - poll_bot_required_checks: polls GitHub's Checks API for the head SHA until every remaining required context completes; PASS iff all conclude success/neutral/skipped, FAIL immediately on any other conclusion, FAIL on timeout naming unresolved contexts. Transient API errors are retried within the same timeout budget. - evaluate_bot_path: orchestrates the above; main() takes its result only when not None, otherwise falls back completely to the existing check(body, head_sha) path -- every --body/--head-sha-only invocation is unchanged. CLI gains --pr-author-login/--pr-author-id/--pr-author-type, --owner/ --repo (defaulting from $GITHUB_REPOSITORY), --trusted-bots-path, --ruleset-path, --head-commit-author-email/--head-commit-committer-email, and --poll-timeout-seconds/--poll-interval-seconds, for Task C's workflow wiring. Adds direct-name-coverage tests plus the defeat tests this issue's ACM and the design doc's own Testing section require: forged id with matching login, an absent required_status_checks rule (fail-closed, not a vacuous pass), a main.json fixture carrying every non-required_status_checks rule type (never misread as check contexts), and GitHub API transient-error retry within the polling budget. The pre-existing 42-test suite for this file passes unmodified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…1858) Follow-up to a2b08d3 after uv run --frozen python3 .github/scripts/gitapex_gate_local_preflight.py surfaced five findings against that commits own diff: - exception-handler-gap: load_trusted_bots/load_ruleset now catch OSError/UnicodeDecodeError at the read_text() boundary itself and re-raise as ValueError, matching gitapex_detect_changed_gate_scripts.pys own registered_gate_paths pattern instead of leaving that boundary unguarded. - except-fail-open: main()s trusted_bots/ruleset loading no longer defaults either to a falsy placeholder and continues -- both loads now share one try/except, and evaluate_bot_path is only ever called from the success branch (both loads succeeded), so a broken/unreadable main.json or trusted-bots.yml skips the bot path entirely rather than running it against placeholder data. The two remaining fail-open sites (the malformed-regex catch in email pattern matching, and the fetch-head-commit-email-failure catch in evaluate_bot_path) are genuinely intentional fall-throughs to the existing human-verdict path per the design docs own Revision section, each disclosed inline with an except-fail-open WAIVED comment naming the reason. - function-body-test-coverage: added direct-name tests for _rule_of_type, _fetch_check_runs, and _latest_run_for_context; the private _sort_key closure (no externally callable name) is disclosed inline with a function-body-test-coverage WAIVED comment, exercised through _latest_run_for_context's own test instead. - detection-logic-property-coverage: added hypothesis property tests for the email-pattern-matching helper to tests/test_gitapex_gate_independent_review_pending_properties.py (starts_with/ends_with/contains match Pythons own semantics, regex matches re.search, unknown operator and empty/non-string pattern always fail closed, arbitrary input never raises). - stdlib-only-claim-drift: removed the literal stale-claim phrase this files own docstring still carried after gaining a real yaml/ _gitapex_github_http import, and switched every Usage example to uv run --frozen python3, matching gitapex_gate_retro_title_convention_citation.pys own precedent. uv run --frozen python3 -m pytest --no-cov -q and .github/scripts/gitapex_gate_local_preflight.py both green after this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… workflow (#1858) Extend .github/workflows/independent-review-pending.yml so the required check can take the bot-merge-gate path Task B's gitapex_gate_independent_review_pending.py already implements: - permissions: add checks:read (workflow- and job-level) for the Checks API poll the bot path needs; contents:read is unchanged. - timeout-minutes: 5 -> 20, headroom over the script's own 900s (15 min) default poll timeout so the job's own GitHub Actions timeout never races the script's timeout FAIL; costs nothing for human/agent PRs, which never reach the polling branch. - Pass github.event.pull_request.user.{login,id,type} to the script as --pr-author-login/--pr-author-id/--pr-author-type via env (never interpolated directly into the shell script, matching the existing PR_BODY convention), plus GITHUB_TOKEN for the script's own GitHub API calls. --owner/--repo are omitted (the script resolves them from $GITHUB_REPOSITORY). --head-commit-author-email/ --head-commit-committer-email are deliberately never passed: they are a test-only override in the script, and wiring them here would let a forged value bypass the real GitHub-API-fetched commit identity the design's own critical-defect fix depends on. Verified: uv run --frozen python3 -m pytest --no-cov -q (8714 passed) and uv run --frozen python3 .github/scripts/gitapex_gate_local_preflight.py (47/47 gates PASS, including independent-review-heading-drift and main-ruleset-required-checks). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deduplicate load_trusted_bots/load_ruleset's identical read-boundary try/except into one shared _read_utf8_or_raise helper, and collapse three copy-pasted is_trusted_bot negative-match tests and two copy-pasted head_commit_identity_matches_bot mismatch tests into pytest.mark.parametrize blocks. Adds three direct tests for the newly extracted _read_utf8_or_raise (required by this repo's own function-body-test-coverage gate for a new named function), including the one UnicodeDecodeError branch that had no prior coverage even before extraction. No production behavior changed: full pytest suite and gitapex_gate_local_preflight.py both green before and after, same pass/fail split for every pre-existing test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rough (issue #1858) evaluate_bot_path's own `except GitHubApiError: ... return None` branch (guarding the head-commit email fetch) was confirmed correct only by code reading -- Codecov reported it as never actually exercised at runtime. Add a direct test: no head-commit email override given (forcing the fetch), an injected opener that makes fetch_head_commit_emails raise GitHubApiError, and an assertion that evaluate_bot_path falls through to the strict human-verdict path (returns None) with the expected stderr note, per the design doc's own Revision section and this exact except clause's own except-fail-open WAIVED comment. No production logic changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sue #1858) drafting-a-pr-to-merge Step 8's own independent review (five parallel axis reviewers against the implemented PR) found two CONFIRMED issues, both verified directly against the actual workflow/CODEOWNERS/gate-script files rather than accepted on the reviewers' own say-so: - load_trusted_bots/load_ruleset read .github/trusted-bots.yml and .github/rulesets/main.json off local disk, which for this pull_request-triggered workflow is the PR's own merge-ref checkout -- a PR could in principle widen its own bot-exemption eligibility by editing either file within its own diff. main.json additionally had no CODEOWNERS protection at all (only trusted-bots.yml did), despite being equally load-bearing for the critical-defect fix (head_commit_identity_matches_bot). Fix: main() now accepts --trust-anchor-ref (the workflow passes github.event.pull_request.base.sha); when given, both files are fetched via the GitHub Contents API (fetch_repo_file_at_ref) from that ref instead of local disk. main.json now also carries the same @tvna CODEOWNERS entry trusted-bots.yml already had. - poll_bot_required_checks cached a required context as permanently "concluded" the first time it observed a passing conclusion, and never looked at it again for the rest of the same poll call -- so a manual re-run into a worse conclusion, while the poll session was still open waiting on another context, would be silently missed. Every context's own conclusion is now re-derived from the latest full check-runs snapshot on every iteration. Design doc updated with a "Second revision" section recording both findings and their fixes. New tests cover fetch_repo_file_at_ref, main()'s --trust-anchor-ref wiring (success and every fallback path), the re-run-after-passing defeat test, and direct coverage for the two newly-extracted _parse_trusted_bots/_parse_ruleset helpers (function-body-test-coverage's own requirement). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qxcd2iHFzURaCjfH9BXvVr
… branch (issue #1858) A second Step 8 independent-review round, run specifically against the previous trust-anchor-ref-pinning fix, found two more issues: - base.sha's own "immutable, no PR ref can move it" premise silently assumed the PR's base.ref stays this repository's real default branch. Since `edited` is one of this workflow's own trigger types, a PR could retarget its own base to a different, possibly branch-protection-free branch carrying forged trusted-bots.yml/main.json content, and the next re-run would fetch trust-anchor content from that forged base instead. Fix: main() now also requires --trust-anchor-base-ref (github.event.pull_request.base.ref) to equal --repo-default-branch (github.event.repository.default_branch -- the repository object's own field, never the PR's) before trusting --trust-anchor-ref at all; any mismatch, or either value unresolved, refuses the bot path the same way every other bot-path failure already does. - An explicitly-given-but-empty --trust-anchor-ref was treated identically to the flag being omitted -- silently falling back to the local-disk read this whole fix exists to stop trusting, with no warning printed at all. Fix: an empty value is now refused with its own explicit error; the silent local-disk fallback is reserved for the flag being omitted entirely. Design doc's "Second revision" section extended with a third item recording both findings and their fixes. New tests cover the base-ref mismatch (the core defeat test), missing base-ref/default-branch, and the empty-string case; existing --trust-anchor-ref tests updated to pass a matching base-ref/default-branch pair. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qxcd2iHFzURaCjfH9BXvVr
tvna
marked this pull request as draft
September 6, 2026 06:55
…ocstrings (issue #1858) The design doc and the gate script's own docstrings/comments had accumulated "Revision"/"Second revision"/"a Step 8 review found..." narration documenting every review round that led to the current design, rather than just describing the current design itself. Consolidated the design doc's Revision/Second revision sections into the current Architecture/Decision logic detail/Residual risks content (351 -> 263 lines), and trimmed the equivalent narration out of the gate script's module docstring, function docstrings, CLI help text, and inline comments (now cross-referencing the design doc's plain "Decision logic detail" section instead of a since-removed "second revision" heading). No behavior change -- docs/comments only. Full suite still 8735 passed; mypy/ruff clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qxcd2iHFzURaCjfH9BXvVr
This was referenced Sep 6, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Exempts trusted bots (Dependabot today) from
independent-review-pending's human-verdict requirement, substituting a machine-checkable condition (all other required status checks green) so Dependabot's own PRs stop being permanently unmergeable. Human- and AI-agent-authored PRs keep the existing strict rule unchanged.Facts
independent-review-pendingis a required status check (.github/rulesets/main.json) that blocks merge until a## Independent review verdictsection naming the PR's exact head commit is recorded in the PR body (issue drafting-a-pr-to-merge: add required independent-review-pending status check to close Step 8 merge-race gap #1311).5880666to062346a#1523, chore(deps): bump openai from 3.3.1 to 3.5.0 #1524, chore(deps): bump prek from 0.4.14 to 0.5.0 #1525, chore(ci): bump astral-sh/setup-uv from 9.0.0 to 10.0.1 #1267), oldest since 2026-08-23, never carry that section and so fail this check permanently.independent-review-pending, which failed, in both..github/rulesets/main.json(PR fix(rulesets): allowlist author/committer email to stop misattribution #1843) already carries Dependabot's GitHub App user id (49699333) inside itscommit_author_email_pattern/committer_email_patternrules, confirming that id as a trustworthy, already-verified identifier for this bot.Assumptions
Acceptance Criteria Map
(restated from issue #1858)
.github/trusted-bots.yml, CODEOWNERS-protected; matched on login+id+type, not login alonemain.json's email-pattern rules; PASS iff every other required check is green; both trust-anchor files are fetched from the PR's base commit (not its own working tree), and that base ref must equal the repo's real default branchparse_verdict/check()pathmain.json's other rule types are never misread as check contextsrequired_status_checksruleDedup: not applicable to a PR body -- issue-level dedup was already disclosed in issue #1858 at creation time.
Risk / blast radius
Scoped to
independent-review-pending's own decision logic and one new config file; human/agent PR review requirements are unaffected. Anti-spoofing (login+id+type, head-commit-email match, trust-anchor-ref pinned to the repo's default branch) limits impersonation to a GitHub-account-level attack, matching this repo's existing single-operator trust model.Rollback
Revert this PR's commits (
git revert). No live branch-protection change is required --main.json's required-checks list itself is unchanged.Verification
uv run --frozen python3 -m pytest --no-cov -q-> 8735 passed.uv run --frozen python3 .github/scripts/gitapex_gate_local_preflight.py-> 47/47 gates PASS (head2b121f0a).2b121f0a) is docs/comments-only (trimmed revision-history narration from the design doc and gate-script docstrings/comments per reviewer feedback) -- no logic change; same 8735 tests, mypy, ruff, and local-preflight all still pass.Checklist
skills/*/SKILL.md, adocs/superpowers/specs/*.mddesign doc, a security-relevant skill, or a deterministic checker script (skills/*/scripts/*.py,evals/scripts/*.py,.github/scripts/*.py), a## Skill audit evidencesection discloses the required verdicts/waivers (see.github/scripts/gitapex_gate_skill_audit_disclosure.py)evals/*/split.md, that entry discloses a Transfer check line (see.github/scripts/gitapex_gate_transfer_check_disclosure.py)skills/*/SKILL.md's Stop-boundary bullets or named dispatch branches,evals/<skill>/tasks/*.yamlgained at least as many new fixtures (see.github/scripts/gitapex_gate_skill_branch_fixture_coverage.py)Skill audit evidence
This PR adds/modifies a design doc (
docs/gitapex/specs/2026-09-06-dependabot-trusted-bot-gate-exemption-design.md) and a deterministic gate script (.github/scripts/gitapex_gate_independent_review_pending.py).design-doc-adversarial-review: RAN -- found and fixed 1 critical defect (bot-identity matched only the PR opener, not the head commit); see the design doc's own Decision logic detail section.checker-script-adversarial-review: RAN -- 8 independentreview-personarounds across implementation and the Step 8 merge review. Confirmed and fixed 4 defects: trust anchors read from the PR's own working tree instead of its base commit;main.jsonhad no CODEOWNERS protection; the poll loop cached a check's conclusion instead of re-checking it; the base ref wasn't verified against the repo's real default branch. Final round found no surviving defects.deterministic-gate-quality: RAN -- every new branch fails closed on malformed or unresolved input (dimension 15); verified with dedicated malformed-input tests, not only happy-path fixtures.defeat-test-disclosure: RAN -- each new detection branch has a test built to defeat it, not just exercise its happy path (seetests/test_gitapex_gate_independent_review_pending.py).Merge gate: independent review
This PR is also subject to the
independent-review-pendingrequiredstatus check (see
.github/workflows/independent-review-pending.yml/.github/scripts/gitapex_gate_independent_review_pending.py). It stayspending/failing until a
## Independent review verdictsection namingthis PR's current head commit is recorded in this body --
drafting-a-pr-to-merge's own Step 8 records it once its independentreview completes. There is nothing for you to do here now: do not
pre-fill this section yourself, and do not remove this note.
Independent review verdict
Related Issue
Closes #1858