Require merged upstream recipe PRs#2210
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - [ ] Verified that every single-node vLLM/SGLang recipe in this PR is documented in the official [vLLM recipes](https://recipes.vllm.ai/) and/or the [SGLang cookbook](https://docs.sglang.io/cookbook/intro): | ||
| - [ ] I linked the corresponding upstream PR in the [vLLM recipe repo](https://github.com/vllm-project/recipes) or [SGLang repo](https://github.com/sgl-project/sglang/tree/main/docs_new) and verified that it is **MERGED** before this InferenceX PR merges. An opened, draft, or closed-without-merge upstream PR does not satisfy this requirement. If the matching recipe was already published, I linked the published recipe/cookbook page in the additional detail section below. |
There was a problem hiding this comment.
🟡 This PR reworks the recipe-link checklist item in docs/PR_REVIEW_CHECKLIST.md to require a merged upstream vLLM/SGLang recipe PR (previously an opened PR was enough), but docs/PR_REVIEW_CHECKLIST_zh.md was not updated to match. AGENTS.md explicitly requires same-PR bilingual mirroring for this exact doc pair, and the Chinese file's template block (lines 29-30) and translated item 10 (line 50) still describe the old, weaker standard, giving Chinese-reading CODEOWNERs stale and now-contradictory guidance.
Extended reasoning...
What changed and what was missed: This PR reworks the recipe-link checklist item in docs/PR_REVIEW_CHECKLIST.md (lines 27-28) from 'recipes are similar to the official ones; if not, a PR has been opened and linked' to 'every recipe is documented upstream; the corresponding upstream PR is linked and verified MERGED before this PR merges.' This is a real policy tightening — it also drives a matching rewrite of .github/codeowner-signoff-verify-prompt.md Check 3, which now hard-fails on open/draft/closed-unmerged upstream PRs. However, docs/PR_REVIEW_CHECKLIST_zh.md was not touched at all.\n\nWhy this violates an explicit repo rule: AGENTS.md states: 'Any edit to an English doc MUST be mirrored in its _zh counterpart (and vice versa) in the same PR — same sections, links, badges, images', and it names docs/PR_REVIEW_CHECKLIST.md/docs/PR_REVIEW_CHECKLIST_zh.md as one of the currently-tracked bilingual pairs. The only carve-outs are agent-instruction files (AGENTS.md/CLAUDE.md/KLAUD_DEBUG.md) and internal .github//utils/ references — this checklist doc is not in that exception list. The sign-off-template carve-out (template text stays English-verbatim in both files) doesn't apply here either — it just means the English wording should be copy-pasted identically into the zh file, not that the zh file's copy is exempt from being updated when the English wording changes.\n\nConcrete divergence after this PR merges, confirmed by reading docs/PR_REVIEW_CHECKLIST_zh.md:\n1. Its English-verbatim template block (lines 29-30) still contains the OLD text: 'Verified that the single-node recipes are similar to the official... If they are not, I have verified that a PR has been opened... and linked it below.' This block is supposed to be identical English in both files, and now it isn't.\n2. Its Chinese translation notes, item 10 (line 50), still reads: '已确认单节点 recipe 与官方...相似；如果不相似，已确认...开了 PR，并...给出链接' — i.e., 'open a PR and link it' is still presented as sufficient. There is no mention of the new MERGED requirement anywhere in the file.\n\nImpact: A Chinese-reading CODEOWNER who follows docs/PR_REVIEW_CHECKLIST_zh.md (which the English doc itself links as the 中文 counterpart via the language switcher at the top of both files) will believe an opened-but-unmerged upstream recipe PR satisfies the requirement, when the actual enforced standard — per the English doc and the rewritten .github/codeowner-signoff-verify-prompt.md Check 3(b) — now requires state: MERGED with a non-null mergedAt. This directly undermines the purpose of the PR for half its audience and leaves two governance docs stating contradictory merge gates.\n\nStep-by-step proof:\n1. Before this PR: EN item 10 said 'similar to official; if not, open+link a PR' — ZH item 10 said the same thing ('相似...开了 PR'). Consistent.\n2. This PR changes ONLY the EN wording to 'documented upstream; linked upstream PR must be MERGED.' ZH file is untouched (diff only touches .github/codeowner-signoff-verify-prompt.md and docs/PR_REVIEW_CHECKLIST.md).\n3. After merge: EN item 10 requires MERGED. ZH item 10 (line 50) still says opening a PR is enough. A Chinese-reading reviewer signs off believing an open PR qualifies.\n4. The automated verifier (.github/codeowner-signoff-verify-prompt.md Check 3(b), also updated in this PR) will independently query the upstream PR's state and FAIL the sign-off if it isn't merged — so the Chinese-reading reviewer's sign-off gets rejected by CI, contradicting what their own checklist told them to expect.\n\nFix: In the same PR, update docs/PR_REVIEW_CHECKLIST_zh.md: mirror the English-verbatim template lines 29-30 to match the new EN template wording, and reword item 10 in the '各条目中文对照说明' section (line 50) to describe the MERGED requirement, consistent with the English update.
| `https://docs.sglang.io/cookbook/...`). If no such link is present, FAIL. | ||
| - (b) MAJOR SERVER ARGS MATCH: Fetch the linked recipe (use the `fetch` MCP tool or | ||
| WebFetch; for a recipe PR, read its diff via `gh pr diff` against that repo if | ||
| accessible) and compare it to this PR's launch command. The recipe only needs to | ||
| match the MAJOR, deployment-defining server args — NOT every flag, and explicitly | ||
| NOT the knobs that are specific to InferenceX benchmark/harness tuning. | ||
| - (b) UPSTREAM CHANGE MERGED: For a linked GitHub PR, query the upstream repository | ||
| directly (for example, `gh pr view <URL> --json state,mergedAt,url`) and require | ||
| `state: MERGED` with a non-null `mergedAt`. An open PR, draft PR, closed-unmerged | ||
| PR, bare branch, or bare commit does NOT pass. A published recipe/cookbook page |
There was a problem hiding this comment.
🟡 Check 3's summary in the workflow's own header comment (.github/workflows/codeowner-signoff-verify.yml, lines 18-20) still says a linked recipe 'PR/commit' satisfies the check, but this PR changes the actual rule to require the linked PR be MERGED (a bare commit link now fails). The comment is stale documentation and should be updated to match, though it has no effect on the bot's actual behavior since the rendered prompt file is correct.
Extended reasoning...
What's stale
.github/workflows/codeowner-signoff-verify.yml (lines 15-33) carries a maintainer-facing header comment enumerating what the bot checks. Line 18-20 currently reads:
# 3. The single-node recipe(s) are linked in the sign-off AND the linked
# recipe PR/commit contains all of the reproduction information that is in
# this InferenceX PR. ...
This PR reworks Check 3 in the rendered prompt template (.github/codeowner-signoff-verify-prompt.md) to add a new sub-check (b) UPSTREAM CHANGE MERGED, requiring gh pr view <URL> --json state,mergedAt to show state: MERGED with a non-null mergedAt, and explicitly states "An open PR, draft PR, closed-unmerged PR, bare branch, or bare commit does NOT pass." The check title itself changes from "Recipe linked AND complete" to "Recipe linked, MERGED, AND complete."
Why this is stale, not just imprecise
The YAML header comment was written to summarize the old Check 3 behavior, where a bare commit link was an acceptable artifact ("recipe PR/commit"). This PR intentionally tightens that behavior — a bare commit link now explicitly fails, and a MERGED state is required. The header comment was not touched, so it now describes a rule that no longer exists: it still tells a maintainer skimming the workflow file that a commit link is fine, and omits the MERGED requirement entirely.
Step-by-step proof
- Before this PR: Check 3(a) said a link to "a PR/commit in ...recipes" satisfies the requirement — matching the YAML comment's "recipe PR/commit" wording.
- This PR's diff to
.github/codeowner-signoff-verify-prompt.mdchanges (a) to require "the corresponding merged recipe PR" (dropping "commit" as acceptable) and adds (b), which states explicitly: "An open PR, draft PR, closed-unmerged PR, bare branch, or bare commit does NOT pass." - Reading the current
.github/workflows/codeowner-signoff-verify.ymllines 18-20 (unchanged by this PR) shows it still says "the linked recipe PR/commit" with no mention of a merge requirement. - Therefore a maintainer who reads only the workflow's header comment (a natural place to look for a quick summary of what the bot enforces) would believe a bare commit link still passes Check 3 — which is now false.
Why nothing catches this
This comment is not covered by AGENTS.md's sync rule (line 86), which only requires docs/PR_REVIEW_CHECKLIST.md to stay in sync with the verifier prompt template. There is no rule mandating this YAML header comment track the rendered prompt, and nothing enforces it automatically (it is a plain # comment, not executable). So it can drift silently, as it has here.
Impact and fix
There is no functional impact — the workflow renders the actual prompt template (which is correctly updated) via envsubst, so the bot's real behavior enforces the new MERGED requirement correctly. The only consequence is that a maintainer skimming the workflow file's summary comment gets an inaccurate mental model of Check 3. The fix is a one-line edit to lines 18-20, replacing "recipe PR/commit" with "merged recipe PR (or published recipe/cookbook page)" and adding a mention of the MERGED requirement.
1 participant
Summary
Verification