Skip to content

[MAINTENANCE] Surface the RFC threshold in the PR template, AGENTS.md, and a new check - #12043

Draft
joshua-stauffer wants to merge 1 commit into
developfrom
m/contrib-pr-rfc-gate
Draft

[MAINTENANCE] Surface the RFC threshold in the PR template, AGENTS.md, and a new check#12043
joshua-stauffer wants to merge 1 commit into
developfrom
m/contrib-pr-rfc-gate

Conversation

@joshua-stauffer

Copy link
Copy Markdown
Collaborator

Summary

@abdullah094 opened #12012 adding Exasol support and got told an RFC was required after writing the code. That's on us — the RFC criteria live in CONTRIBUTING.md, and nothing in the pull-request path pointed at them. I said on that PR I'd fix it for the future; this is that fix.

What changed

1. Pull-request template — states the RFC threshold inline (in a comment above the checklist, so the criteria are visible while writing the description) and adds a checklist item for it.

Two drive-by fixes in the same file:

  • The AGENTS.md link was relative, so it resolved to .github/AGENTS.md (404) rather than the file at the repo root. Now absolute.
  • Remaining github.com/great-expectations/... links normalized to fivetran/..., matching CONTRIBUTING.md and the rest of .github/.

2. AGENTS.md gains a "Before opening a pull request" section. This is the part that matters for tooling. gh pr create --body discards the pull-request template entirely, and nothing injects it into a coding agent's context — so a template-only fix reaches humans opening PRs in the web UI and misses every programmatically-opened PR. AGENTS.md is the file agents read from the repo root by convention, which makes it the highest-leverage place to put this.

3. New pr-hygiene check (.github/workflows/pr-hygiene.yml).

Flags changes that look like new backend support and asks for one line in the description — either RFC: <link> or No RFC needed: <reason>. Signals, all added-file only, so modifying an existing datasource doesn't trip it:

Signal Means
new great_expectations/compatibility/*.py new optional third-party dependency
new great_expectations/datasource/fluent/*_datasource.py new fluent datasource
new reqs/requirements-dev-*.txt new backend requirements

Design notes:

  • It enforces that the question was answered, never what the answer is. A false positive costs one line in the description, not a blocked PR. That keeps it useful without making it adversarial.
  • When it trips, it posts a comment explaining what to do (once, marker-guarded against re-posting). A failing check annotation alone doesn't carry enough context for a first-time contributor — and a prescriptive failure message is also what lets a coding agent self-correct in one iteration.
  • pull_request_target, metadata-only. It reads PR metadata through the API and never checks out or executes PR code, so it's safe on fork PRs. That trigger is the point: it fires the moment a PR opens, rather than waiting for a maintainer to approve a CI run — which is exactly the delay that made [FEATURE] Exasol integration #12012 go wrong.
  • It also checks the title prefix, exempting bot authors whose titles are generated upstream.

4. Removes the legacy Markdown issue templates. bug_report.md and feature_request.md were superseded by the structured issue forms but never deleted, so GitHub kept offering all four in the chooser. Because the Markdown templates carry labels: '', issues filed through them arrived with neither a type label nor triage — silently bypassing the triage queue that the ready-for-work claiming gate depends on.

Verification

  • pre-commit run --files ... — all hooks pass (check-yaml, prettier, end-of-file-fixer, trailing-whitespace)
  • Workflow YAML parses; embedded script passes node --check
  • Signal regexes and the body scanner unit-tested against 19 cases, including the real [FEATURE] Exasol integration #12012 file list (trips all three signals) and the unedited template (correctly does not pass — the scanner strips HTML comments first, otherwise the template's own guidance text would satisfy the check)
  • ci/checks/check_repo_root_size.sh passes (no root files added)

Not done here

  • pr-hygiene is not yet a required check. It's a separate workflow, so it can't join ci-required's needs: list — that would also put it behind the actor-permission gate and destroy the early-feedback property. Making it blocking is a branch-protection setting; I'd suggest living with it advisory for a few weeks first to see the false-positive rate.
  • It won't run on this PR. pull_request_target uses the workflow from the base branch, so it goes live on the first PR opened after merge.

A contributor opening a pull request for new data source support had no way to
learn an RFC was expected: the criteria live in CONTRIBUTING.md, and nothing in
the pull-request path pointed at them. The cost lands on the contributor, who
finds out only after writing the code.

Three changes, aimed at the three places that signal can land:

- The pull-request template now states the RFC threshold inline and carries a
  checklist item for it. It also fixes the AGENTS.md link, which used a relative
  path and resolved to .github/AGENTS.md rather than the file at the repo root,
  and normalizes the remaining github.com links onto the current repository path.

- AGENTS.md gains a "Before opening a pull request" section. Tools that compose a
  pull-request body programmatically never load the template, so a template-only
  fix reaches humans and misses everything else; AGENTS.md is the file those tools
  do read.

- A new pr-hygiene check flags changes that look like new backend support (a new
  compatibility module, a new fluent datasource, or a new reqs/requirements-dev-*
  file) and asks for one line in the description: either a link to an accepted RFC
  or a statement that none is needed. It enforces that the question was answered,
  never what the answer is, so a false positive costs one line rather than a
  blocked pull request. It reads pull-request metadata through the API and never
  checks out pull-request code, so it can run on fork pull requests immediately
  instead of waiting for a maintainer to approve a CI run.

Also removes the legacy Markdown issue templates. They were superseded by the
structured issue forms but left in place, so GitHub kept offering all four in the
chooser. Because the Markdown templates apply no labels, issues filed through them
arrived with neither a type label nor `triage`, silently bypassing the triage
queue that the ready-for-work claiming gate depends on.
@netlify

netlify Bot commented Aug 7, 2026

Copy link
Copy Markdown

Deploy Preview for niobium-lead-7998 canceled.

Name Link
🔨 Latest commit a74dfdd
🔍 Latest deploy log https://app.netlify.com/projects/niobium-lead-7998/deploys/6a75e1b2b9565000080d7df4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant