fix(reuse): accept a pinned cancelled sweep run as a reuse source - #2389
Conversation
A fail-fast sweep cancels its remaining jobs, so a run whose benchmark jobs all passed still concludes cancelled once a later job is cut short. The pinned path already allows an incomplete source run because ingestion skips rows without results, but it only permitted success and failure, so those sweeps could not be pinned at all.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested fix that extends the existing pinned-reuse allowance to include cancelled alongside failure, matching the documented fail-fast-cancellation scenario.
Extended reasoning...
Overview
This PR touches only utils/find_reusable_sweep_run.py and its test file. The change adds cancelled to the set of conclusions accepted for a pinned reusable sweep source run (alongside the existing success/failure), with a clear rationale: a fail-fast sweep whose benchmark jobs all passed can still conclude cancelled if a later, unrelated job (e.g. an eval-only job) is cut short. The unpinned path is untouched and still requires success only, so the blast radius of the change is scoped to the explicit maintainer-opt-in /reuse-sweep-run <run_id> path.
Security risks
None identified. This is CI-internal tooling that decides whether a GitHub Actions run's artifacts can be reused for ingestion; it doesn't touch auth, secrets, or user-facing surfaces. The existing artifact-presence check (has_reusable_result_artifacts) and PR-commit-membership check are unaffected, so a pinned cancelled run still has to have real, unexpired result artifacts and a head SHA present in the PR's commit list before it can be reused.
Level of scrutiny
Low-to-moderate. This is an internal CI/ops utility, not a production inference code path, and the change is a narrow, single-condition allowlist extension with symmetric new tests (accept pinned cancelled, reject unpinned cancelled) added alongside the existing failure tests. The logic mirrors an already-reviewed pattern (the failure case) rather than introducing new design.
Other factors
Test coverage is solid — the PR adds two new unit tests following the same structure as existing failure-case tests, and the description states the full suite (26 tests) passes, including a manual validation against the real run cited in the motivation. The one issue surfaced by the bug-hunting pass is a metadata nit (missing bilingual PR title/description per AGENTS.md), which doesn't affect code correctness or CI behavior, so it doesn't change my assessment of the code itself.
| raise RuntimeError(f"Reusable source run {run_id} is not a pull_request run.") | ||
| if run.get("status") != "completed": | ||
| raise RuntimeError(f"Reusable source run {run_id} is not completed.") | ||
| allowed_conclusions = {"success", "failure"} if allow_failed else {"success"} | ||
| # A pinned run is an explicit maintainer choice, so incomplete sweeps are | ||
| # allowed: ingestion skips rows without results, leaving only the completed | ||
| # points. ``cancelled`` belongs here alongside ``failure`` because a | ||
| # fail-fast sweep cancels its remaining jobs, so a run whose benchmark jobs | ||
| # all passed still concludes ``cancelled`` when a later job is cut short. | ||
| allowed_conclusions = ( | ||
| {"success", "failure", "cancelled"} if allow_failed else {"success"} | ||
| ) | ||
| if run.get("conclusion") not in allowed_conclusions: | ||
| expected = "success or failure" if allow_failed else "success" | ||
| expected = ( | ||
| "success, failure, or cancelled" if allow_failed else "success" | ||
| ) | ||
| raise RuntimeError( | ||
| f"Reusable source run {run_id} has conclusion {run.get('conclusion')!r}; " | ||
| f"expected {expected}." |
There was a problem hiding this comment.
🟡 This PR's title and description are English-only, but AGENTS.md (line 7) explicitly requires bilingual PR titles/descriptions — title format <English title> / <中文标题> and a Chinese section (e.g. ## 中文说明) in the body. Please add the missing Simplified Chinese translation to both the title and description.
Extended reasoning...
What the bug is: AGENTS.md — the file CLAUDE.md explicitly delegates all repo guidance to — states an unambiguous, mandatory policy at line 7:
PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format:
<English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a## 中文说明section mirroring the summary; don't translate code blocks, logs, or stack traces — summarize around them). ... This applies to every PR and every issue, matching the bilingual docs rule in Code Conventions.
This is not a stylistic suggestion; it's phrased as a hard requirement ("must be bilingual") that applies to "every PR."
How it manifests here: PR #2389's title is fix(reuse): accept a pinned cancelled sweep run as a reuse source — English only, with no / <中文标题> suffix. Its description is likewise entirely in English, with no ## 中文说明 (or equivalent Chinese) section anywhere in the body.
Why nothing else in the PR catches this: the code diff and tests in utils/find_reusable_sweep_run.py / utils/test_find_reusable_sweep_run.py are unaffected by this rule — AGENTS.md carves out an explicit exception for agent-instruction files and internal .github//utils/ references being English-only, but that exception is scoped to documentation files, not to PR metadata itself. The bilingual requirement for PR titles/descriptions is repo-wide and independent of which files are touched, so this PR is not exempt just because it modifies a Python utility.
Proof/verification: I grepped AGENTS.md directly and confirmed line 7 contains the exact bilingual mandate quoted above, applying to "every PR." Comparing against recently merged PRs in this same repo's git log confirms the policy is actively enforced in practice, e.g. commit d121d11 [AgentX]: Use burst phase starts by default / [AgentX]：默认使用阶段起始突发模式 (#2375) and 4837735 [NV] Refresh MiniMax-M3 NVFP4 B300 8k1k disaggregated Dynamo-vLLM / 更新 MiniMax-M3 NVFP4 B300 8k1k 分离式 Dynamo-vLLM 配置 (#2310) — both follow the <English> / <中文> title format. PR #2389's title and body do not follow this pattern anywhere, confirming the violation.
Impact and fix: This has no effect on code correctness, the reuse-run validation logic, or CI — it's purely a metadata/compliance gap. The fix is simply to update the PR title to fix(reuse): accept a pinned cancelled sweep run as a reuse source / <中文标题> and append a ## 中文说明 section translating the summary to natural technical Chinese (per the translation quality bar also defined in AGENTS.md), before merge.
1 participant
validate_reusable_runallows an incomplete source run only when the run wasexplicitly pinned with
/reuse-sweep-run <run_id>, because ingestion skips rowswithout results and therefore lands only the completed points. That allowance
covered
successandfailurebut notcancelled.cancelledis the conclusion a fail-fast sweep reaches whenever a job is cutshort: the remaining jobs are cancelled, and the run concludes
cancelledevenwhen every benchmark job passed and uploaded artifacts. Those runs could not be
pinned at all, so a maintainer had no way to recover their completed points.
Concrete case: PR #2371's sweep
run 30326393603
has all 12
agenticjobssuccesswith 12bmk_agentic_*artifacts, but oneeval-onlyjob was cancelled, cascading totrigger-ingest— so the runconcluded
cancelledand the #2371 ingest could not be recovered(#2386 is blocked on this).
Unpinned reuse is unchanged and still requires
success.Verified:
pytest utils/test_find_reusable_sweep_run.py→ 26 passed, includingthe two new cases (pinned
cancelledaccepted, unpinnedcancelledrejected),plus a direct
validate_reusable_runcall against the real run 30326393603 andPR #2386 commit list.