[potiuk]

Stacked on #74825. Part 6 of the AIP-120 implementation (design: #73378); this PR's base is the previous branch, so the diff shows only this layer.

Follow-ups to the reconciler in #74822, one commit each, plus protection of the gate itself:

• A "push again" nudge is posted before the state comment records awaiting_push, so a failed post is retried.
• A workflow or trigger the base branch does not have counts as project CI in the tampering check only if one of its jobs ran.
• Pull requests sharing a head commit across different bases stay green_unverified, since one commit status cannot hold a decision per base.
• Recorded runs made while the pull request targeted a base without the gate are ignored; a run that could belong to another pull request from the same branch is reported to Slack instead of closing.
• A gated pull request that changes the fork CI gate (gate code, configuration beyond the author's own [pilot] entry, the gate job or its wiring in ci-amd.yml, or the monitor) is closed with a comment explaining why. Each of those files says so. The state comment's marker records gate_checked only when this check ran for the head commit, so pull requests reconciled before it existed are checked once.

closes: #74905
closes: #74906
closes: #74907
closes: #74908

---

Was generative AI tooling used to co-author this PR?

• Yes — Claude Code (Opus 5)

Generated-by: Claude Code (Opus 5) following the guidelines

🤖 Generated with Claude Code

[muditagrawal2007]

Stacked-on-#74825 design is clean, and the "post the closing comment before the state comment records the new state" change is the right call — a failed post was silently lost before this PR. The gate-protection added layer is also a sensible guardrail for the same class of self-deciding risk that motivated the original gate. Read the full diff against fork-ci-5-docs; below is what stood out.

1. The gate-modified marker doesn't record gate_checked: true

The closure paths for gate_bypassed, closed (inactivity) and the regular state branches all now write Marker(..., gate_checked=gate_checked) — but the new gate_modified branch in release/classify writes Marker(ForkState.GATE_MODIFIED.value, pr.head_sha, now) without that flag:

_set_marker_comment(
    plan,
    pr,
    state_comment(
        Marker(ForkState.GATE_MODIFIED.value, pr.head_sha, now), fork=pr.head_repo, run_url=None
    ),
)

The documentation states the opposite:

The state comment's marker records "gate_checked": true with the head commit only when this check ran for it

The check did run for this head commit — that's why we're in this branch. So the comment and the doc disagree.

The reason this matters in practice is maintainer-reopen-without-restore. Reading the docstring for reopened_since_closing, when marker.state == gate_modified and pr.last_reopened_at > marker.since, the "reopen since closing" path is skipped (correctly), and execution falls through to the gate-check block. With gate_checked: false on the marker, the check re-runs and closes the PR again — which matches the documented intent:

a maintainer's reopen without restoring the gate closes it again on the next run

So either:

• The code is right and the doc sentence above is overstated (e.g., add an exception for "and the close path retains gate_checked: false so a reopen re-runs the check"), or
• The code should record gate_checked: true and already_checked should additionally require pr.last_reopened_at is None or pr.last_reopened_at <= marker.since. That keeps the once-per-head-commit guarantee and still re-runs after a maintainer reopen.

The second option is more consistent with the rest of the diff and with the rule "checked once per head commit". Right now the rule is held together by the same exception that makes the doc misleading.

already_checked = (
    marker is not None
    and marker.gate_checked
    and marker.sha == pr.head_sha
    and (pr.last_base_change_at is None or pr.last_base_change_at < marker.since)
    and (pr.last_reopened_at is None or pr.last_reopened_at <= marker.since)
)

Then record gate_checked=gate_checked on the gate-modified marker too, and add a test for the reopen-without-restore case (test_gate_change_reopens_closes_again or similar).

2. Gate-modified plans don't ask for the gate_checked sentinel that other closures carry

Same point as #1, expressed from the consumer side. test_a_change_to_the_gate_closes only asserts '"state": "gate_modified"' in plan.marker_comment, which passes whether or not the gate-checked flag is set. Tightening that to "gate_checked": true would lock in whichever resolution you pick and surface regressions in either direction. The other three tests for this branch (test_unfinished_gate_change_closure_is_retried, test_marker_records_whether_the_gate_was_checked, test_gate_change_is_checked_once_per_head_commit) sidestep the close path.

3. classify is now ~270 lines doing everything

Counting from the start of classify to its return plan, the function decides: which paths skip the gate (use project ci/use fork ci), gate tampering, gate-modified closure, "reopen since closing", fork state observation, green-trust decision (unverified vs verified), the shared-head-across-bases path, draft/labels, status, marker. None of these are individually complicated but reading them as one diff is. A small refactor — extracting _plan_closure(state, comment_factory, *, change=...) for the three close paths and _plan_record_state(state, since) for the record-only paths, with classify as a top-level orchestrator — would make the diff much easier to review and the existing tests can stay because they assert on Plan shape.

4. observe_fork_state doesn't know about gate_modified

def observe_fork_state(fork: ForkObservation, marker: Marker | None) -> tuple[ForkState | None, str | None]:
    ...
    if fork.workflow_state is None:
        if marker is not None and marker.state == ForkState.AWAITING_PUSH.value:
            return ForkState.AWAITING_PUSH, None
        return None, "no fork run and the fork workflow state was not read"

For a re-opened gate_modified PR with no fork run, this returns (None, "no fork run...") rather than (GATE_MODIFIED, None). That's tolerable because the release/gate-check block runs before observe_fork_state and either closes again or falls through (depending on #1), but if you do end up extending the gate-check block to be re-entrant from observe_fork_state, the function needs an explicit gate_modified case, otherwise the post-#1 architecture risks an unknown line per run.

5. _other_pull_requests is re-issued per run, not cached per branch

BypassFinder._other_pull_requests calls rest_get(pulls?head=...) with pagination. The result depends only on (head_repo, head_ref, window_start) and find() is invoked once per branch by run_reconcile. Hoisting it into BypassFinder.__init__ (computed once with the same 6h window) would save one paginated call per PR per run. Trivial cost per call, but the call pattern repeats across every reconciles and shows up in the GitHub calls: {...} log line.

6. _set_status URL is hardcoded in the new branch

_set_status(
    plan, pr, ForkState.GATE_MODIFIED.value, f"https://github.com/{repo}/pull/{pr.number}/files"
)

vs. the helper used elsewhere — fork.run_url or fork_actions_url(pr.head_repo) and f"https://github.com/{repo}/pull/{pr.number}/checks". The hardcoded value is fine semantically (a gate_modified PR has no fork run to link to; linking the changed files is the right thing), but the inconsistency is worth a helper for consistency — something like gate_changed_files_url(pr.number) if you don't want to inline it.

7. _list_jobs raises and goes through the except GitHubError path

try:
    jobs = self._list_jobs(run)
except GitHubError:
    result = True
else:
    result = ...

That result = True on the right-hand side of the OR-test for "did work" reads the wrong way at first glance — "no jobs listed → counts as project CI" is exactly the intended semantic, but the previous shape made the same point by setting result = True before checking anything and the diff moved it under a guard. Worth a two-line comment at the assignment:

except GitHubError:
    # A listing we can't make fails open: better to flag a run that turned out
    # to be project CI than to miss one.
    result = True

Two of the parametrized tests cover the False branch; there isn't a test that exercises rest_get raising on the jobs call. Adding one would lock in the "fail open on listing error" semantics.

8. base_at boundary semantics

for changed_at, previous, _ in reversed(pr.base_changes):
    if when < changed_at:
        base = previous

For a run whose created_at exactly equals a BaseRefChangedEvent.createdAt (when == changed_at), the new base applies. That's right for a run triggered by the retarget itself (pull_request with edited: change_base), which sees the new base. For a run whose created_at is one millisecond less than the retarget (when < changed_at), the loop sets base = previous (old base). That's also defensible — the run was triggered against the old base. The asymmetry around the boundary is intentional but worth a one-line comment; "runs at or after a retarget are considered to be against the new base".

9. Schema test for MONITORED_GATE_PROTECTED_PATHS?

GATE_PROTECTED_PATHS is a single frozenset at module scope; if a future PR renames a file or adds another gate piece (e.g., fork_ci_maintainer.py) the maintainer who lands it needs to remember to add it both here and to the four "this file is part of the gate" comments. A small assert that mirrors the comments — pulling the strings out of those four header comments and checking every protected path has a back-reference — would catch a forgotten update. Optional, but it's the kind of doc/code drift the existing comments are trying to prevent.

10. Smaller things

• Marker(ForkState.GATE_MODIFIED.value, pr.head_sha, now) is built both inline (state_comment call) and via a future helper — at this PR's size, the duplication is small but a const for the per-state string would help.
• from dataclasses import replace — is_own_pilot_change imports it but doesn't use replace directly... wait, it does: return replace(head, pilot=base.pilot) == base and .... Fine.
• PILOT_BASE = "enabled = true\n[pilot]\nother = 2026-10-01T00:00:00Z\n" lives at module scope in the test file. Minor — fine for tests, but if anyone wants a second file to reference "what does an unchanged base look like" they'll copy-paste.
• The "Gate protection" section in 08_fork_ci_for_external_contributors.md is excellent and the table-row addition plus the **New comments go first.** rule are real, easily-understood improvements.

Overall the diff lines up with its stated design, the test density matches the risk of each new branch (≥6 cases for "checked once per head commit" alone), and the gate tampering and shared-head paths are clearly motivated by #74905 / #74906 / #74907. The one thing I'd want resolved before merge is the gate-modified marker flag conflict in #1/#2 — whichever way you settle it, it's worth a one-line comment where the marker is built explaining why gate_checked is on every other closure's marker and the exception.