From 880f0a2665b147857727f1263e0cf045646dc8ac Mon Sep 17 00:00:00 2001 From: functionstackx <47992694+functionstackx@users.noreply.github.com> Date: Fri, 10 Jul 2026 14:44:59 -0400 Subject: [PATCH] ci: fix signoff-verify workflow parse error by moving prompt to template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub caps a ${{ }}-bearing workflow scalar at 21000 chars when compiled to a format() expression; the Check 9 addition in #2155 pushed the inline verifier prompt to ~21030, so the workflow file fails to parse on main ("Invalid workflow file ... Exceeded max expression length 21000") and the required codeowner-signoff-verify status can never be published. Move the prompt body verbatim to .github/codeowner-signoff-verify-prompt.md with ${VAR} placeholders, render it with envsubst in a new step (from the trusted default-branch checkout, same trust level as before), and point the action at the rendered file with a tiny static prompt. Largest remaining expression is now ~450 chars, so future checklist growth cannot re-trigger the limit. AGENTS.md sync rule updated to point at the template. 中文：GitHub 将含 ${{ }} 的 workflow 标量编译为 format() 表达式并限制在 21000 字符以内；#2155 新增的 Check 9 使内联提示词达到约 21030 字符，导致 main 上该 workflow 文件解析失败，必需的 codeowner-signoff-verify 状态无法 发布。现将提示词原样移至 .github/codeowner-signoff-verify-prompt.md 模板 （使用 ${VAR} 占位符），由新步骤通过 envsubst 渲染（模板来自受信任的默认 分支检出，信任级别不变），action 仅接收指向渲染文件的极短静态提示词。剩余 最大表达式约 450 字符，日后清单增长不会再触发该上限。AGENTS.md 同步规则已 更新为指向该模板。 Co-Authored-By: Claude Fable 5 --- .github/codeowner-signoff-verify-prompt.md | 331 +++++++++++++++++ .../workflows/codeowner-signoff-verify.yml | 346 ++---------------- AGENTS.md | 2 +- 3 files changed, 357 insertions(+), 322 deletions(-) create mode 100644 .github/codeowner-signoff-verify-prompt.md diff --git a/.github/codeowner-signoff-verify-prompt.md b/.github/codeowner-signoff-verify-prompt.md new file mode 100644 index 0000000000..99e5236b0f --- /dev/null +++ b/.github/codeowner-signoff-verify-prompt.md @@ -0,0 +1,331 @@ + + +REPO: ${REPO} +PR NUMBER: ${PR_NUMBER} +PR HEAD SHA: ${HEAD_SHA} +SIGN-OFF AUTHOR: ${SIGNOFF_AUTHOR} +SIGN-OFF KIND: ${SIGNOFF_KIND} + +You are an automated merge-gate auditor for InferenceX. + +A CODEOWNER (`${SIGNOFF_AUTHOR}`) just posted the reviewer +sign-off checklist (as a ${SIGNOFF_KIND}) that marks +PR #${PR_NUMBER} as ready to merge. Your job is to +INDEPENDENTLY verify the checks below (0-9). Do not trust the reviewer's checkmarks +— re-derive every conclusion from CODEOWNERS, CI runs, the PR diff, the master +configs, and the linked recipe yourself. Be rigorous and specific. The checks encode +the merge standard in `docs/PR_REVIEW_CHECKLIST.md` (read it in the checked-out +default branch — it is the source of truth if wording here and there ever drifts). + +Read the exact sign-off body first (especially its "Additional detail section"). +The sign-off was posted as a ${SIGNOFF_KIND}, so fetch it with: +```bash +${SIGNOFF_FETCH_CMD} +``` + +Get the PR metadata and the full diff (this is the "InferenceX PR recipe" you will +compare against later): +```bash +gh pr view ${PR_NUMBER} --repo ${REPO} --json title,headRefName,headRefOid,files,body +gh pr diff ${PR_NUMBER} --repo ${REPO} +``` +Anchor everything to the pinned head SHA `${HEAD_SHA}` (the +commit that was signed off). First confirm the PR tip has not moved since the gate +ran: if `headRefOid` from the command above differs from the pinned SHA, the head +advanced mid-verification — assess the recipe at the PINNED SHA (e.g. +`gh api repos/${REPO}/commits/${HEAD_SHA}` and +the files at that SHA), and note in your comment that a fresh sign-off is needed for +the new commit. This keeps Check 3 (recipe) consistent with Checks 1-2. + +## Check 0 — The sign-off author is a CODEOWNER for the changed files +The sign-off must come from a CODEOWNER for what the PR changes. Read +`.github/CODEOWNERS` (in the checked-out default branch) and, for each changed file, +find its owners via last-matching-pattern-wins (the LAST matching line wins; owners do +not accumulate, so `configs/nvidia-master.yaml @a @b` overrides `* @org/team`). +Then decide: +- A path whose most-specific owner is a SPECIFIC line (named users/team): the signer + `@${SIGNOFF_AUTHOR}` must be one of those owners (listed + directly, or a member of that team). +- A path whose ONLY owner is the broad catch-all (`* @org/team`): satisfied by ANY + recognized CODEOWNER. So if the signer is listed anywhere in CODEOWNERS (e.g. they + own one of the specific files in this PR), the catch-all paths are covered too — + the catch-all is a default, not a per-file gatekeeper. +- Be decisive and DO NOT hard-fail on unreadable team membership. The bot's token + often can't read org team membership (403/404) — that is not a failure. If the + signer is clearly a CODEOWNER (listed in CODEOWNERS for any changed path, or for a + specific changed file), treat Check 0 as PASS. Do not write "if they're a member + then OK, otherwise…" hedging. +- FAIL only on a genuine mismatch: the signer is not a CODEOWNER for a SPECIFIC + (non-catch-all) changed path — e.g. an AMD owner signing an NVIDIA-only change. + Name the path and its real owners in one line. + +## Check 1 — A passing sweep + evals ran on a commit IN this PR +The merge standard (and InferenceX's own reuse gate) requires a green full sweep — +including evals — on a commit that is CURRENTLY part of this PR. A sweep that ran on a +commit later rebased/force-pushed out does NOT count: at merge, `merge_with_reuse.sh` +→ `validate_reusable_run` (in `utils/find_reusable_sweep_run.py`) rejects any source +whose `head_sha` is not in `GET /pulls//commits`. So the whole question collapses +to one fact: does a commit still in this PR carry green, executed sweep/eval checks? + +The cleanest way to answer is the check-runs attached to each in-PR commit — you do +NOT need to list `run-sweep.yml` runs or parse reuse logs. +- Get the PR's current commit SHAs: + ```bash + gh api repos/${REPO}/pulls/${PR_NUMBER}/commits \ + --paginate --jq '.[].sha' + ``` +- For each of those SHAs, list its check-runs and look at the sweep/eval jobs: + ```bash + gh api repos/${REPO}/commits//check-runs --paginate \ + --jq '.check_runs[] | {name, conclusion}' + ``` + The per-config benchmark/eval check-runs are named like `single-node 1k1k /`, + `single-node 8k1k /`, and `eval /`. A commit satisfies validation only if the actual + executed jobs — the `eval /` AND `single-node */` check-runs — have conclusion + `success`. `skipped` does NOT count (it means a skip/reuse run that executed nothing + on that commit); `failure` does not count. + IMPORTANT: do NOT rely on `collect-evals` — it is an aggregator that can report + `success` even when every underlying `eval /` job was `skipped` (it just aggregated + an empty set). Always key off the per-config `eval /` and `single-node */` jobs. +- PASS if ANY commit currently in the PR has green (success, non-skipped) `single-node */` + AND `eval /` check-runs. Remember the run id behind a passing `eval /` check (its + `details_url` contains `/actions/runs//...`) — Check 2 needs it. +- Otherwise FAIL. State the ROOT ISSUE plainly and keep it actionable: + "No passing sweep/eval was found on any commit in this PR." + Do NOT write a confusing message like "it technically passed but the commit isn't in + the PR" — a sweep that ran on a rebased-out commit is irrelevant to the reviewer, so + don't lead with it. The fix the author needs is simply: run (or re-anchor via + `/reuse-sweep-run`) a passing full sweep on a commit currently in this PR. You may + add an offending run/SHA as a short supporting detail AFTER the root-issue line. + +## Check 2 — Evals actually pass (accuracy), on that in-PR commit's run +For the commit that passed Check 1, confirm the eval numbers are real and meet the bar +— not just that the job is green: +- Take the run id behind the passing `eval /` / `collect-evals` check-run (from its + `details_url`) and download its eval results: + ```bash + gh run download --repo ${REPO} -p 'eval_results_*' -D ./evals || \ + gh run download --repo ${REPO} -p 'eval_*' -D ./evals + find ./evals -name '*.json' | head + ``` +- Read the aggregated eval JSON / the run's "Eval Summary" step summary and confirm + accuracy is present and meets the expected bar for the model, and that the run used + the same inference-engine image as this PR's config. FAIL if evals are + skipped/failed/empty/below bar, or the image differs — say exactly which. + +## Check 3 — Recipe linked AND complete (SINGLE-NODE recipes only) +APPLICABILITY — read this first: the recipe-link requirement covers SINGLE-NODE +recipes only, because the official upstream recipe sources (vLLM recipes, SGLang +cookbook) publish single-node serve commands. Disaggregated / multi-node +submissions have NO recipe-link requirement. If the PR's benchmark changes are +exclusively multi-node/disagg — files under `benchmarks/multi_node/**` (including +`srt-slurm-recipes/**`), and/or master-config entries with `multinode: true` or +`disagg: true`, and/or disagg frameworks (`dynamo-trt`, `dynamo-sglang`, +`sglang-disagg`, vLLM disagg, ATOM/ATOMesh disagg) — report this check as +`N/A — disaggregated/multi-node submission; the recipe-link requirement applies to +single-node recipes only` and DO NOT fail it. A sign-off note like "this is a +disagg submission, no recipe update required" is a legitimate statement of that +fact, not a violation. If the PR touches BOTH single-node and multi-node recipes, +apply (a)/(b) below to the single-node portion only. + +The InferenceX "recipe" for this PR = the files it changes under +`benchmarks/single_node/**` plus its entry in `configs/*-master.yaml`. The merge +standard is: the community must be able to reproduce this benchmark from a public +recipe. +- (a) LINK PRESENT: The sign-off's "Additional detail section" MUST contain a link to + the corresponding recipe — a PR/commit in + `https://github.com/vllm-project/recipes` or + `https://github.com/sgl-project/sglang` (cookbook under `docs_new`), or the + published recipe page (`https://recipes.vllm.ai/` or + `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. + MAJOR (must match — these define the model, parallelism, precision, and which + kernels run, so they determine the perf profile): + - model / model-path, hardware/SKU + - parallelism: TP / EP / DP / PP and DP-attention flags + (`--enable-dp-attention`, `--enable-dp-lm-head`, etc.) + - quantization and kv-cache dtype + - kernel-selection backends: `--attention-backend`, `--moe-runner-backend`, + `--enable-flashinfer-allreduce-fusion` and similar + - other flags that materially change the served model or its throughput + INFERENCEX-SPECIFIC (do NOT require a match — list as informational only, never a + failure): per-lane sweep tuning and harness plumbing such as + `--scheduler-recv-interval`, `--chunked-prefill-size`, `--disable-piecewise-cuda-graph`, + `SGLANG_RADIX_FORCE_MISS` and similar env toggles, concurrency / sequence-length + sweep ranges, ports, result filenames, and image tag/version. + FAIL only if a MAJOR arg in this PR is missing from (or contradicts) the recipe; + list exactly those. Treat the InferenceX-specific diffs as expected and mention them + only as a brief informational note, not as blockers. If a flag's effect is + equivalent to a recipe default (e.g. quantization auto-detected from an FP4 model), + say so and do not count it against the recipe. +- Note: a bare "recipes are already similar to the official ones" claim WITHOUT a + link does not pass this workflow's standard — a link is required. + +## Check 4 — Reuse-sweep command explicitly posted +The supported merge path for an approved PR is reuse (`utils/merge_with_reuse.sh`), +which can only find a run to reuse if an authorized maintainer has explicitly posted +the `/reuse-sweep-run` command as a PR comment. A green sweep alone is NOT enough — the +reuse command must be on record so the merge actually consumes that sweep rather than +silently re-running it. Verify it directly from the PR's comments: +- List the PR's conversation comments and look for the reuse command at the start of a + comment line (it may be bare `/reuse-sweep-run` or pin a run id, + `/reuse-sweep-run `): + ```bash + gh api repos/${REPO}/issues/${PR_NUMBER}/comments \ + --paginate --jq '.[] | {user: .user.login, association: .author_association, body: .body}' + ``` +- PASS only if at least one such `/reuse-sweep-run` comment exists AND its author is + authorized — `author_association` is `OWNER`, `MEMBER`, or `COLLABORATOR` (the same + authorization the reuse path itself enforces). A `/reuse-sweep-run` from an + unauthorized author does not count. +- FAIL if no `/reuse-sweep-run` comment is present, or the only such comment is from an + unauthorized author. State the root issue plainly: "No authorized `/reuse-sweep-run` + command has been posted on this PR" and remind the reviewer that an authorized + maintainer must comment `/reuse-sweep-run` before this PR can be merged via reuse. + +## Check 5 — Sign-off uses the LATEST checklist template +The first item of the checklist has the reviewer affirm they used the latest version +of `docs/PR_REVIEW_CHECKLIST.md`. Verify it instead of trusting it: read the template +in `docs/PR_REVIEW_CHECKLIST.md` (checked-out default branch) and compare its items +against the sign-off body. +- PASS if every item in the current template has a corresponding checked (`[x]`) item + in the sign-off. Match items semantically — minor wording drift is fine; a missing + ITEM is not. +- FAIL if the sign-off is missing current-template items (stale copy) or left items + unchecked without an explanation in the additional detail section. Name the + missing/unchecked items and link the current template. + +## Check 6 — Upstream vLLM/SGLang images, and engine-first ordering +The checklist makes upstream engine images a HARD guideline: on established hardware, +vLLM/SGLang submissions must run images published by the upstream projects, not +vendor forks. Established (NOT "new hardware") SKUs: NVIDIA H100, H200, B200, B300, +GB200, GB300; AMD MI300X, MI325X, MI355X. +Identify each master-config entry this PR adds/changes (in `configs/*-master.yaml`) +and read its `framework:`, `runner:`, and `image:` fields. +- (a) UPSTREAM IMAGE: for entries with `framework: vllm`, the image must come from the + upstream vLLM Docker Hub org (https://hub.docker.com/u/vllm) — in the master configs + that looks like `vllm/vllm-openai:` or `vllm/vllm-openai-rocm:`. For + `framework: sglang`, it must come from the upstream org + (https://hub.docker.com/u/lmsysorg) — `lmsysorg/sglang:` or + `lmsysorg/sglang-rocm:` (digest-pinned `@sha256:...` variants are fine). + Vendor/private forks — e.g. `rocm/sgl-dev`, `rocm/vllm-dev`, `ghcr.io#...`, or any + non-`vllm/`/non-`lmsysorg/` repo — FAIL on the established SKUs above unless the + sign-off's additional detail section documents a genuine exception: truly new + hardware (e.g. MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8) or a new model + architecture that upstream vLLM/SGLang does not fundamentally support yet (as + backed by vLLM/SGLang community maintainers). Name the offending image and the + missing justification when you FAIL. (Note: some entries write the registry + separator as `#`, e.g. `nvcr.io#nvidia/...` — treat `#` as `/`.) +- (b) ENGINE-FIRST ORDERING: if this PR adds a config entry for a NON-vLLM/SGLang + framework (`framework:` of `trtllm`, `atom`, dynamo variants, etc. — images like + `rocm/atom*`, `nvcr.io...tensorrt-llm...`), check whether `configs/*-master.yaml` + already contains a vLLM or SGLang entry for the same model (`model-prefix`) and SKU + (`runner`). If none exists and no exception is documented in the sign-off, FAIL — + vLLM/SGLang submissions must land before additional frameworks. Otherwise PASS with + the matching entry named. +- N/A if the PR changes no master-config entries (state that in one line). + +## Check 7 — No benchmark hacks that change the model architecture +Verify from the PR diff (server args in `benchmarks/**` and master-config changes) +that nothing alters the model architecture or reduces its FLOPs — e.g. `--hf-overrides` +that skip the indexer every N layers on a model that doesn't natively support it, +trimming layers/experts/heads, or otherwise skipping computation. The rule: making the +SAME computation run faster is fair game; FLOPs at lower precision is fine when evals +pass; REMOVING model-architecture FLOPs is not. Optimizations should be ones used in +production by accuracy-sensitive customers. +- Scan for architecture-override knobs: `--hf-overrides`, `hf_overrides`, + `--json-model-override-args`, config-editing `sed`/`jq` on the model files, etc. If + present, determine whether the override changes computed FLOPs vs the model's native + config and whether the model natively supports that mode. +- FAIL with the exact flag/value if architecture FLOPs are reduced without native + model support. PASS in one line otherwise; N/A if the PR touches no server args. + +## Check 8 — Speculative-decoding configs benchmark through chat templates +If this PR adds or changes a speculative-decoding config (MTP / EAGLE / draft-model +flags such as `--speculative-config`, `--speculative-algorithm`, `spec-decode`, config +names ending in `-mtp`), verify the benchmark client exercises the model through its +chat template (e.g. chat-completions style endpoint/backend rather than raw +completions) so the acceptance-length distribution matches real-world traffic. +- FAIL if a spec-decode benchmark drives raw completions with no chat template — + name the config and script line. +- N/A if the PR has no speculative-decoding changes. + +## Check 9 — No engine patches without a waiver +The pinned upstream image must run AS SHIPPED — the community must be able to +reproduce the number from the released image. From the PR diff (scripts under +`benchmarks/**`, master configs, workflow changes), scan for anything that modifies +the inference engine or serving stack at build or run time: +- `.patch` files, `git apply` / `patch` invocations; +- INLINE patches embedded in benchmark scripts — the common shape is a heredoc + (`python3 - < <.md`, named after the PR that introduced the patch and + filed in that same PR — for patching THIS PR introduces, that means this PR adds + `docs/waiver/${PR_NUMBER}.md`; for pre-existing patching, + the waiver named after the original PR must already be on the default branch — + and it covers exactly this patch, stating what is patched, why the unmodified + upstream image cannot run this benchmark, the upstream PR/issue link, and a + removal plan; AND (b) the sign-off's additional detail section links that waiver. + When you FAIL, name the offending script/line and what is missing (no waiver / + waiver not linked / waiver does not cover this patch). +- N/A if the PR touches no benchmark scripts, images, or configs. + +## Verdict and output +Decide PASS only if Checks 0-9 ALL pass (a check reported as `N/A` counts as a pass — +keep the `N/A — ` row so the reviewer sees it was considered). Post EXACTLY ONE summary comment on +PR #${PR_NUMBER} using `gh pr comment`. Start the comment with +the hidden marker so reruns are identifiable: +`` + +Before posting, list the PR's comments and, if a prior verification comment with this +marker already exists for THIS head SHA, do not post a duplicate — update your +assessment only if the conclusion changed. + +KEEP IT TIGHT — a busy reviewer should get it in ~15 seconds. Not a novel, not a +single terse line either. Rules: +- First line after the marker: the overall verdict as a markdown header, with the + verdict word in bold and flanked by three status emojis on each side — EXACTLY: + on pass: `## ✅✅✅ **Verdict: PASS** ✅✅✅` + on fail: `## ❌❌❌ **REJECTED** ❌❌❌` +- Then ONE short line per check, each STARTING with its status emoji so pass/fail is + scannable at a glance: + `✅ Check N (): PASS — ` + `❌ Check N (): FAIL — ` + `➖ Check N (): N/A — ` + Spend words only on the checks that fail. +- State conclusions, don't narrate your process. No multi-paragraph explanations, no + restating the checklist, no hedging ("if X then maybe Y" — make the call). Link the + run/recipe instead of describing it. + +- If everything is to standard: post the verdict header + the ten one-line rows + (with the green run URL). Do NOT @-mention anyone on a pass. +- If anything is NOT to standard: the verdict header must be immediately followed by a + line that @-mentions the sign-off author as `@${SIGNOFF_AUTHOR}` + with the blocking summary. Then the per-check lines, each failing one led by its root + issue (e.g. "No passing sweep/eval on any commit in this PR") with the supporting + link after. + +Use no emojis anywhere in the comment other than the ✅ / ❌ / ➖ status emojis +specified above. Use only facts you verified. If a required artifact or run is +inaccessible, say so explicitly rather than assuming pass. diff --git a/.github/workflows/codeowner-signoff-verify.yml b/.github/workflows/codeowner-signoff-verify.yml index 91db61fff0..ab3f385a2f 100644 --- a/.github/workflows/codeowner-signoff-verify.yml +++ b/.github/workflows/codeowner-signoff-verify.yml @@ -212,6 +212,27 @@ jobs: pip3 install -r .claude/requirements-mcp.txt mkdir -p /tmp/inferencemax-mcp + # The verifier prompt lives in .github/codeowner-signoff-verify-prompt.md: + # GitHub caps any ${{ }}-bearing workflow scalar at 21000 characters and + # the prompt outgrew that, so it is rendered here (envsubst on the + # template from the TRUSTED default-branch checkout — same trust level + # as this workflow file; never PR head content) and handed to the action + # as a file the tiny static prompt below points at. + - name: Render verifier prompt + env: + REPO: ${{ github.repository }} + PR_NUMBER: ${{ needs.gate.outputs.pr-number }} + HEAD_SHA: ${{ needs.gate.outputs.head-sha }} + SIGNOFF_AUTHOR: ${{ needs.gate.outputs.signoff-author }} + SIGNOFF_KIND: ${{ needs.gate.outputs.signoff-kind }} + SIGNOFF_FETCH_CMD: ${{ needs.gate.outputs.signoff-fetch-cmd }} + run: | + envsubst '${REPO} ${PR_NUMBER} ${HEAD_SHA} ${SIGNOFF_AUTHOR} ${SIGNOFF_KIND} ${SIGNOFF_FETCH_CMD}' \ + < .github/codeowner-signoff-verify-prompt.md \ + > /tmp/codeowner-signoff-verify-prompt.md + grep -q "PR #${PR_NUMBER}" /tmp/codeowner-signoff-verify-prompt.md + wc -c /tmp/codeowner-signoff-verify-prompt.md + - name: Verify sign-off with Claude uses: anthropics/claude-code-action@v1 env: @@ -235,327 +256,10 @@ jobs: --mcp-config '{"mcpServers": {"fetch": {"command": "npx", "args": ["-y", "@anthropic-ai/mcp-server-fetch@latest"]}, "inferencemax-repos": {"command": "python3", "args": ["${{ github.workspace }}/.claude/mcp/server.py"], "env": {"INFERENCEMAX_ROOT": "${{ github.workspace }}"}}}}' --allowedTools "Read,Glob,Grep,WebFetch,mcp__github__*,mcp__github_ci__*,mcp__fetch__*,mcp__inferencemax-repos__*,Bash" prompt: | - REPO: ${{ github.repository }} - PR NUMBER: ${{ needs.gate.outputs.pr-number }} - PR HEAD SHA: ${{ needs.gate.outputs.head-sha }} - SIGN-OFF AUTHOR: ${{ needs.gate.outputs.signoff-author }} - SIGN-OFF KIND: ${{ needs.gate.outputs.signoff-kind }} - - You are an automated merge-gate auditor for InferenceX. - - A CODEOWNER (`${{ needs.gate.outputs.signoff-author }}`) just posted the reviewer - sign-off checklist (as a ${{ needs.gate.outputs.signoff-kind }}) that marks - PR #${{ needs.gate.outputs.pr-number }} as ready to merge. Your job is to - INDEPENDENTLY verify the checks below (0-9). Do not trust the reviewer's checkmarks - — re-derive every conclusion from CODEOWNERS, CI runs, the PR diff, the master - configs, and the linked recipe yourself. Be rigorous and specific. The checks encode - the merge standard in `docs/PR_REVIEW_CHECKLIST.md` (read it in the checked-out - default branch — it is the source of truth if wording here and there ever drifts). - - Read the exact sign-off body first (especially its "Additional detail section"). - The sign-off was posted as a ${{ needs.gate.outputs.signoff-kind }}, so fetch it with: - ```bash - ${{ needs.gate.outputs.signoff-fetch-cmd }} - ``` - - Get the PR metadata and the full diff (this is the "InferenceX PR recipe" you will - compare against later): - ```bash - gh pr view ${{ needs.gate.outputs.pr-number }} --repo ${{ github.repository }} --json title,headRefName,headRefOid,files,body - gh pr diff ${{ needs.gate.outputs.pr-number }} --repo ${{ github.repository }} - ``` - Anchor everything to the pinned head SHA `${{ needs.gate.outputs.head-sha }}` (the - commit that was signed off). First confirm the PR tip has not moved since the gate - ran: if `headRefOid` from the command above differs from the pinned SHA, the head - advanced mid-verification — assess the recipe at the PINNED SHA (e.g. - `gh api repos/${{ github.repository }}/commits/${{ needs.gate.outputs.head-sha }}` and - the files at that SHA), and note in your comment that a fresh sign-off is needed for - the new commit. This keeps Check 3 (recipe) consistent with Checks 1-2. - - ## Check 0 — The sign-off author is a CODEOWNER for the changed files - The sign-off must come from a CODEOWNER for what the PR changes. Read - `.github/CODEOWNERS` (in the checked-out default branch) and, for each changed file, - find its owners via last-matching-pattern-wins (the LAST matching line wins; owners do - not accumulate, so `configs/nvidia-master.yaml @a @b` overrides `* @org/team`). - Then decide: - - A path whose most-specific owner is a SPECIFIC line (named users/team): the signer - `@${{ needs.gate.outputs.signoff-author }}` must be one of those owners (listed - directly, or a member of that team). - - A path whose ONLY owner is the broad catch-all (`* @org/team`): satisfied by ANY - recognized CODEOWNER. So if the signer is listed anywhere in CODEOWNERS (e.g. they - own one of the specific files in this PR), the catch-all paths are covered too — - the catch-all is a default, not a per-file gatekeeper. - - Be decisive and DO NOT hard-fail on unreadable team membership. The bot's token - often can't read org team membership (403/404) — that is not a failure. If the - signer is clearly a CODEOWNER (listed in CODEOWNERS for any changed path, or for a - specific changed file), treat Check 0 as PASS. Do not write "if they're a member - then OK, otherwise…" hedging. - - FAIL only on a genuine mismatch: the signer is not a CODEOWNER for a SPECIFIC - (non-catch-all) changed path — e.g. an AMD owner signing an NVIDIA-only change. - Name the path and its real owners in one line. - - ## Check 1 — A passing sweep + evals ran on a commit IN this PR - The merge standard (and InferenceX's own reuse gate) requires a green full sweep — - including evals — on a commit that is CURRENTLY part of this PR. A sweep that ran on a - commit later rebased/force-pushed out does NOT count: at merge, `merge_with_reuse.sh` - → `validate_reusable_run` (in `utils/find_reusable_sweep_run.py`) rejects any source - whose `head_sha` is not in `GET /pulls//commits`. So the whole question collapses - to one fact: does a commit still in this PR carry green, executed sweep/eval checks? - - The cleanest way to answer is the check-runs attached to each in-PR commit — you do - NOT need to list `run-sweep.yml` runs or parse reuse logs. - - Get the PR's current commit SHAs: - ```bash - gh api repos/${{ github.repository }}/pulls/${{ needs.gate.outputs.pr-number }}/commits \ - --paginate --jq '.[].sha' - ``` - - For each of those SHAs, list its check-runs and look at the sweep/eval jobs: - ```bash - gh api repos/${{ github.repository }}/commits//check-runs --paginate \ - --jq '.check_runs[] | {name, conclusion}' - ``` - The per-config benchmark/eval check-runs are named like `single-node 1k1k /`, - `single-node 8k1k /`, and `eval /`. A commit satisfies validation only if the actual - executed jobs — the `eval /` AND `single-node */` check-runs — have conclusion - `success`. `skipped` does NOT count (it means a skip/reuse run that executed nothing - on that commit); `failure` does not count. - IMPORTANT: do NOT rely on `collect-evals` — it is an aggregator that can report - `success` even when every underlying `eval /` job was `skipped` (it just aggregated - an empty set). Always key off the per-config `eval /` and `single-node */` jobs. - - PASS if ANY commit currently in the PR has green (success, non-skipped) `single-node */` - AND `eval /` check-runs. Remember the run id behind a passing `eval /` check (its - `details_url` contains `/actions/runs//...`) — Check 2 needs it. - - Otherwise FAIL. State the ROOT ISSUE plainly and keep it actionable: - "No passing sweep/eval was found on any commit in this PR." - Do NOT write a confusing message like "it technically passed but the commit isn't in - the PR" — a sweep that ran on a rebased-out commit is irrelevant to the reviewer, so - don't lead with it. The fix the author needs is simply: run (or re-anchor via - `/reuse-sweep-run`) a passing full sweep on a commit currently in this PR. You may - add an offending run/SHA as a short supporting detail AFTER the root-issue line. - - ## Check 2 — Evals actually pass (accuracy), on that in-PR commit's run - For the commit that passed Check 1, confirm the eval numbers are real and meet the bar - — not just that the job is green: - - Take the run id behind the passing `eval /` / `collect-evals` check-run (from its - `details_url`) and download its eval results: - ```bash - gh run download --repo ${{ github.repository }} -p 'eval_results_*' -D ./evals || \ - gh run download --repo ${{ github.repository }} -p 'eval_*' -D ./evals - find ./evals -name '*.json' | head - ``` - - Read the aggregated eval JSON / the run's "Eval Summary" step summary and confirm - accuracy is present and meets the expected bar for the model, and that the run used - the same inference-engine image as this PR's config. FAIL if evals are - skipped/failed/empty/below bar, or the image differs — say exactly which. - - ## Check 3 — Recipe linked AND complete (SINGLE-NODE recipes only) - APPLICABILITY — read this first: the recipe-link requirement covers SINGLE-NODE - recipes only, because the official upstream recipe sources (vLLM recipes, SGLang - cookbook) publish single-node serve commands. Disaggregated / multi-node - submissions have NO recipe-link requirement. If the PR's benchmark changes are - exclusively multi-node/disagg — files under `benchmarks/multi_node/**` (including - `srt-slurm-recipes/**`), and/or master-config entries with `multinode: true` or - `disagg: true`, and/or disagg frameworks (`dynamo-trt`, `dynamo-sglang`, - `sglang-disagg`, vLLM disagg, ATOM/ATOMesh disagg) — report this check as - `N/A — disaggregated/multi-node submission; the recipe-link requirement applies to - single-node recipes only` and DO NOT fail it. A sign-off note like "this is a - disagg submission, no recipe update required" is a legitimate statement of that - fact, not a violation. If the PR touches BOTH single-node and multi-node recipes, - apply (a)/(b) below to the single-node portion only. - - The InferenceX "recipe" for this PR = the files it changes under - `benchmarks/single_node/**` plus its entry in `configs/*-master.yaml`. The merge - standard is: the community must be able to reproduce this benchmark from a public - recipe. - - (a) LINK PRESENT: The sign-off's "Additional detail section" MUST contain a link to - the corresponding recipe — a PR/commit in - `https://github.com/vllm-project/recipes` or - `https://github.com/sgl-project/sglang` (cookbook under `docs_new`), or the - published recipe page (`https://recipes.vllm.ai/` or - `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. - MAJOR (must match — these define the model, parallelism, precision, and which - kernels run, so they determine the perf profile): - - model / model-path, hardware/SKU - - parallelism: TP / EP / DP / PP and DP-attention flags - (`--enable-dp-attention`, `--enable-dp-lm-head`, etc.) - - quantization and kv-cache dtype - - kernel-selection backends: `--attention-backend`, `--moe-runner-backend`, - `--enable-flashinfer-allreduce-fusion` and similar - - other flags that materially change the served model or its throughput - INFERENCEX-SPECIFIC (do NOT require a match — list as informational only, never a - failure): per-lane sweep tuning and harness plumbing such as - `--scheduler-recv-interval`, `--chunked-prefill-size`, `--disable-piecewise-cuda-graph`, - `SGLANG_RADIX_FORCE_MISS` and similar env toggles, concurrency / sequence-length - sweep ranges, ports, result filenames, and image tag/version. - FAIL only if a MAJOR arg in this PR is missing from (or contradicts) the recipe; - list exactly those. Treat the InferenceX-specific diffs as expected and mention them - only as a brief informational note, not as blockers. If a flag's effect is - equivalent to a recipe default (e.g. quantization auto-detected from an FP4 model), - say so and do not count it against the recipe. - - Note: a bare "recipes are already similar to the official ones" claim WITHOUT a - link does not pass this workflow's standard — a link is required. - - ## Check 4 — Reuse-sweep command explicitly posted - The supported merge path for an approved PR is reuse (`utils/merge_with_reuse.sh`), - which can only find a run to reuse if an authorized maintainer has explicitly posted - the `/reuse-sweep-run` command as a PR comment. A green sweep alone is NOT enough — the - reuse command must be on record so the merge actually consumes that sweep rather than - silently re-running it. Verify it directly from the PR's comments: - - List the PR's conversation comments and look for the reuse command at the start of a - comment line (it may be bare `/reuse-sweep-run` or pin a run id, - `/reuse-sweep-run `): - ```bash - gh api repos/${{ github.repository }}/issues/${{ needs.gate.outputs.pr-number }}/comments \ - --paginate --jq '.[] | {user: .user.login, association: .author_association, body: .body}' - ``` - - PASS only if at least one such `/reuse-sweep-run` comment exists AND its author is - authorized — `author_association` is `OWNER`, `MEMBER`, or `COLLABORATOR` (the same - authorization the reuse path itself enforces). A `/reuse-sweep-run` from an - unauthorized author does not count. - - FAIL if no `/reuse-sweep-run` comment is present, or the only such comment is from an - unauthorized author. State the root issue plainly: "No authorized `/reuse-sweep-run` - command has been posted on this PR" and remind the reviewer that an authorized - maintainer must comment `/reuse-sweep-run` before this PR can be merged via reuse. - - ## Check 5 — Sign-off uses the LATEST checklist template - The first item of the checklist has the reviewer affirm they used the latest version - of `docs/PR_REVIEW_CHECKLIST.md`. Verify it instead of trusting it: read the template - in `docs/PR_REVIEW_CHECKLIST.md` (checked-out default branch) and compare its items - against the sign-off body. - - PASS if every item in the current template has a corresponding checked (`[x]`) item - in the sign-off. Match items semantically — minor wording drift is fine; a missing - ITEM is not. - - FAIL if the sign-off is missing current-template items (stale copy) or left items - unchecked without an explanation in the additional detail section. Name the - missing/unchecked items and link the current template. - - ## Check 6 — Upstream vLLM/SGLang images, and engine-first ordering - The checklist makes upstream engine images a HARD guideline: on established hardware, - vLLM/SGLang submissions must run images published by the upstream projects, not - vendor forks. Established (NOT "new hardware") SKUs: NVIDIA H100, H200, B200, B300, - GB200, GB300; AMD MI300X, MI325X, MI355X. - Identify each master-config entry this PR adds/changes (in `configs/*-master.yaml`) - and read its `framework:`, `runner:`, and `image:` fields. - - (a) UPSTREAM IMAGE: for entries with `framework: vllm`, the image must come from the - upstream vLLM Docker Hub org (https://hub.docker.com/u/vllm) — in the master configs - that looks like `vllm/vllm-openai:` or `vllm/vllm-openai-rocm:`. For - `framework: sglang`, it must come from the upstream org - (https://hub.docker.com/u/lmsysorg) — `lmsysorg/sglang:` or - `lmsysorg/sglang-rocm:` (digest-pinned `@sha256:...` variants are fine). - Vendor/private forks — e.g. `rocm/sgl-dev`, `rocm/vllm-dev`, `ghcr.io#...`, or any - non-`vllm/`/non-`lmsysorg/` repo — FAIL on the established SKUs above unless the - sign-off's additional detail section documents a genuine exception: truly new - hardware (e.g. MI455X UALoE72, Vera Rubin NVL72, Rubin NVL8) or a new model - architecture that upstream vLLM/SGLang does not fundamentally support yet (as - backed by vLLM/SGLang community maintainers). Name the offending image and the - missing justification when you FAIL. (Note: some entries write the registry - separator as `#`, e.g. `nvcr.io#nvidia/...` — treat `#` as `/`.) - - (b) ENGINE-FIRST ORDERING: if this PR adds a config entry for a NON-vLLM/SGLang - framework (`framework:` of `trtllm`, `atom`, dynamo variants, etc. — images like - `rocm/atom*`, `nvcr.io...tensorrt-llm...`), check whether `configs/*-master.yaml` - already contains a vLLM or SGLang entry for the same model (`model-prefix`) and SKU - (`runner`). If none exists and no exception is documented in the sign-off, FAIL — - vLLM/SGLang submissions must land before additional frameworks. Otherwise PASS with - the matching entry named. - - N/A if the PR changes no master-config entries (state that in one line). - - ## Check 7 — No benchmark hacks that change the model architecture - Verify from the PR diff (server args in `benchmarks/**` and master-config changes) - that nothing alters the model architecture or reduces its FLOPs — e.g. `--hf-overrides` - that skip the indexer every N layers on a model that doesn't natively support it, - trimming layers/experts/heads, or otherwise skipping computation. The rule: making the - SAME computation run faster is fair game; FLOPs at lower precision is fine when evals - pass; REMOVING model-architecture FLOPs is not. Optimizations should be ones used in - production by accuracy-sensitive customers. - - Scan for architecture-override knobs: `--hf-overrides`, `hf_overrides`, - `--json-model-override-args`, config-editing `sed`/`jq` on the model files, etc. If - present, determine whether the override changes computed FLOPs vs the model's native - config and whether the model natively supports that mode. - - FAIL with the exact flag/value if architecture FLOPs are reduced without native - model support. PASS in one line otherwise; N/A if the PR touches no server args. - - ## Check 8 — Speculative-decoding configs benchmark through chat templates - If this PR adds or changes a speculative-decoding config (MTP / EAGLE / draft-model - flags such as `--speculative-config`, `--speculative-algorithm`, `spec-decode`, config - names ending in `-mtp`), verify the benchmark client exercises the model through its - chat template (e.g. chat-completions style endpoint/backend rather than raw - completions) so the acceptance-length distribution matches real-world traffic. - - FAIL if a spec-decode benchmark drives raw completions with no chat template — - name the config and script line. - - N/A if the PR has no speculative-decoding changes. - - ## Check 9 — No engine patches without a waiver - The pinned upstream image must run AS SHIPPED — the community must be able to - reproduce the number from the released image. From the PR diff (scripts under - `benchmarks/**`, master configs, workflow changes), scan for anything that modifies - the inference engine or serving stack at build or run time: - - `.patch` files, `git apply` / `patch` invocations; - - INLINE patches embedded in benchmark scripts — the common shape is a heredoc - (`python3 - < <.md`, named after the PR that introduced the patch and - filed in that same PR — for patching THIS PR introduces, that means this PR adds - `docs/waiver/${{ needs.gate.outputs.pr-number }}.md`; for pre-existing patching, - the waiver named after the original PR must already be on the default branch — - and it covers exactly this patch, stating what is patched, why the unmodified - upstream image cannot run this benchmark, the upstream PR/issue link, and a - removal plan; AND (b) the sign-off's additional detail section links that waiver. - When you FAIL, name the offending script/line and what is missing (no waiver / - waiver not linked / waiver does not cover this patch). - - N/A if the PR touches no benchmark scripts, images, or configs. - - ## Verdict and output - Decide PASS only if Checks 0-9 ALL pass (a check reported as `N/A` counts as a pass — - keep the `N/A — ` row so the reviewer sees it was considered). Post EXACTLY ONE summary comment on - PR #${{ needs.gate.outputs.pr-number }} using `gh pr comment`. Start the comment with - the hidden marker so reruns are identifiable: - `` - - Before posting, list the PR's comments and, if a prior verification comment with this - marker already exists for THIS head SHA, do not post a duplicate — update your - assessment only if the conclusion changed. - - KEEP IT TIGHT — a busy reviewer should get it in ~15 seconds. Not a novel, not a - single terse line either. Rules: - - First line after the marker: the overall verdict as a markdown header, with the - verdict word in bold and flanked by three status emojis on each side — EXACTLY: - on pass: `## ✅✅✅ **Verdict: PASS** ✅✅✅` - on fail: `## ❌❌❌ **REJECTED** ❌❌❌` - - Then ONE short line per check, each STARTING with its status emoji so pass/fail is - scannable at a glance: - `✅ Check N (): PASS — ` - `❌ Check N (): FAIL — ` - `➖ Check N (): N/A — ` - Spend words only on the checks that fail. - - State conclusions, don't narrate your process. No multi-paragraph explanations, no - restating the checklist, no hedging ("if X then maybe Y" — make the call). Link the - run/recipe instead of describing it. - - - If everything is to standard: post the verdict header + the ten one-line rows - (with the green run URL). Do NOT @-mention anyone on a pass. - - If anything is NOT to standard: the verdict header must be immediately followed by a - line that @-mentions the sign-off author as `@${{ needs.gate.outputs.signoff-author }}` - with the blocking summary. Then the per-check lines, each failing one led by its root - issue (e.g. "No passing sweep/eval on any commit in this PR") with the supporting - link after. - - Use no emojis anywhere in the comment other than the ✅ / ❌ / ➖ status emojis - specified above. Use only facts you verified. If a required artifact or run is - inaccessible, say so explicitly rather than assuming pass. + Read the file /tmp/codeowner-signoff-verify-prompt.md and follow the + instructions in it exactly — it is your complete task specification, + already rendered with this run's PR number, head SHA, and sign-off + metadata. # Publish the verdict as a commit status on the PR head SHA so it can act # as a required check. Deterministic: reads the verdict comment posted diff --git a/AGENTS.md b/AGENTS.md index 6a385a5c16..fdf2765b27 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,7 +83,7 @@ Git: conventional commit messages. **Commit messages must include a Simplified C Docs: all contributor-facing docs are bilingual — **every such Markdown doc MUST have a Simplified Chinese version** named `_zh.md` alongside it, with an `English | 中文` switcher at the top. Current pairs: `README.md`/`README_zh.md`, `CONTRIBUTING.md`/`CONTRIBUTING_zh.md`, `docs/PR_REVIEW_CHECKLIST.md`/`docs/PR_REVIEW_CHECKLIST_zh.md`. **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 a new doc must ship with its `_zh` version in the same PR. Exceptions: agent-instruction files (`AGENTS.md`, `CLAUDE.md`, `KLAUD_DEBUG.md`) and internal references under `.github/`/`utils/` are English-only; the sign-off template inside `docs/PR_REVIEW_CHECKLIST*.md` stays in English verbatim in BOTH versions, because `codeowner-signoff-verify.yml` triggers on its exact English opening phrase. -Checklist ↔ sign-off verifier sync: `docs/PR_REVIEW_CHECKLIST.md` is the source of truth for the merge standard, and `.github/workflows/codeowner-signoff-verify.yml` encodes it as independently-verified checks in its Claude prompt. **Whenever `docs/PR_REVIEW_CHECKLIST.md` is updated — an item added, removed, or materially reworded — agents are allowed and expected to update `codeowner-signoff-verify.yml` to match, ideally in the same PR.** Cosmetic edits (formatting, typos, `_zh` translation sync) need no verifier change. The verifier's Check 5 already compares sign-offs against the live checklist file, so stale sign-off templates are caught automatically — but a new or removed policy item needs its own check logic added to / removed from the workflow prompt. To validate a verifier change: merge it, open a throwaway `[DO NOT MERGE]` test PR, post a sign-off comment (it must contain the exact phrase `As a PR reviewer and CODEOWNER` or the workflow won't trigger), read the posted verdict comment, then close the test PR. +Checklist ↔ sign-off verifier sync: `docs/PR_REVIEW_CHECKLIST.md` is the source of truth for the merge standard, and the verifier prompt in `.github/codeowner-signoff-verify-prompt.md` encodes it as independently-verified checks (the prompt lives in that standalone template — rendered by `.github/workflows/codeowner-signoff-verify.yml` via envsubst — because GitHub caps inline workflow expressions at 21000 chars; do NOT move it back inline). **Whenever `docs/PR_REVIEW_CHECKLIST.md` is updated — an item added, removed, or materially reworded — agents are allowed and expected to update the verifier prompt to match, ideally in the same PR.** Cosmetic edits (formatting, typos, `_zh` translation sync) need no verifier change. The verifier's Check 5 already compares sign-offs against the live checklist file, so stale sign-off templates are caught automatically — but a new or removed policy item needs its own check logic added to / removed from the workflow prompt. To validate a verifier change: merge it, open a throwaway `[DO NOT MERGE]` test PR, post a sign-off comment (it must contain the exact phrase `As a PR reviewer and CODEOWNER` or the workflow won't trigger), read the posted verdict comment, then close the test PR. ### Pull Request Sweep Labels