Repository navigation
perf(minimax-m3): tune prefill index score for gfx1250 - #2764
Merged
Merged
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
XiaobingSuper
force-pushed
the
xiaobingsuper/m3-index-score-gfx1250
branch
from
October 27, 2026 13:41
d911c4d to
be613ac
Compare
XiaobingSuper
changed the base branch from
lirui/m3-gfx1250-flydsl-pa-decode
to
main
October 27, 2026 13:41
zufayu
added a commit
that referenced
this pull request
Oct 27, 2026
T5 ("accumulator precision loses tail-of-sum bits") was carried from aiter
with no ATOM instance from the start, and P1's 26-PR distribution measurement
found it fired 0/14 times across all Triton-gated PRs -- it cannot answer
"which real ATOM change would have gone unreviewed without this rule".
Re-searched ATOM's git log and merged-PR history for an fp16/bf16-accumulator
precision-loss fix or review comment before removing it; found none (the
closest hits -- e.g. PR #2764's FP32-accumulator register-pressure tuning --
are a different defect, not tail-of-sum precision loss). Removed from
ALL_RULES, derive()/triton_families(), and rules.md's body (a removal note
explaining the grading replaces the rule body, matching this branch's
convention of saying why a rule isn't there rather than leaving a silent
gap). Not moved to UNREACHABLE_BY_DESIGN -- that list is for evidence that
arrives by another route, not a parking spot for a rule that never earned
its place. MAPPING.md regenerated via `triage.py mapping`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
valarLip
previously approved these changes
Oct 28, 2026
Refine PR #2764's one-stage FP8 launch from eight wave32 warps to four. Keep the existing BF16, gfx950 and decode launch policies unchanged; score arithmetic, causal masking and selection are untouched. Add architecture/dtype policy tests and exact score/top-k/sparse-table regression coverage for ragged requests, zero-query requests, query strides, ties and matched prefill shapes. Validation: 172 passed, 1 skipped on each of two MI455 GPUs; ruff and black pass. Isolated same-machine matched FP8 score latency versus prior PR head: 56.997 to 40.268 us (one local head), 199.975 to 132.290 us (four heads). No new end-to-end model or GSM8K result is claimed.
valarLip
approved these changes
Oct 28, 2026
zufayu
added a commit
that referenced
this pull request
Nov 12, 2026
…tom-bot (P0) (#2762) * docs: atom-bot PR review pipeline design Port the aiter review-pr + validate-kernel-pr pipeline to ATOM as atom-bot, triggered by @atom-bot review with its own workflow in this repo. Develop and test on srdc-24 (8x MI308X idle, review agent on a remote GLM gateway so the GPUs stay free for validation); box308 is the eventual target but its 8 GPUs are currently held by a local GLM server. Key decisions: GPU sharing uses a default subset with fallback to the full idle ranking; the minimal e2e case compares base-vs-head delta rather than absolute scores, because 200-sample GSM8K carries +-3pp at 2 sigma against a 2pp threshold margin; the ATOM rule set is regrown from real ATOM review history rather than ported verbatim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: close the four open questions in the atom-bot design Config splits three ways instead of the over-corrected 'everything but secrets in version control': committing the GPU subset would have meant an upstream PR to change which cards our own box may use. The rejected alternative is recorded so it is not proposed again. Model backend is two-phase: the AMD gateway while the flow is being built on srdc-24, box308's already-running GLM-5.3 in the end. That demotes gateway quota to a development-speed risk, but it promotes box308's GPU occupancy to a must-answer: box308 is the final target and its eight cards are held by that GLM, so validate has no GPU there. Keep validate independently invocable. Budgets: 120 min run inside a 150 min job, because validate serves a model twice in the same run. Whether those two rounds fit at all is unmeasured, and measuring it is the first action of P2, before any wiring is written. P1 corpus: six months of every public ATOM comment channel, no reviewer allowlist, with a rule admitted only when it can name the real ATOM PR that would go unreviewed without it. The bot gets its own selftest CI lane rather than sharing the repo's: the main CI gates on absolute accuracy across PRs, the bot compares base against head within one PR, and on conflict the main CI wins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: P0 implementation plan for the atom-bot review chain Nine tasks, each ending in an independently testable deliverable and a commit. The ordering is by dependency, not by file size: the gates land first because they are what makes a skipped step visible, and fetch.sh only after the deriver exists, since an empty rules.txt makes the ledger gate fail closed and every later failure a false one. Two corrections from the self-review are already folded in. ATOM's HK numbers differ in meaning from aiter's at the same number, so copying aiter's mapping would have produced verdicts pointing at the wrong rule body -- which no gate can catch, since they only check that every derived rule has a verdict. And STEP4 needed a body in rules.md: triage.py expand hard-fails on a rule id that can be derived but has no text, which would have broken Task 4, not Task 9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: vendor the gate infrastructure from aiter _lib.py, _gates.py, _collect.py and _apply_refutation.py carry no repo-specific logic, so they port unchanged apart from one comment example. The contract test pins the gate roster and the required-artifact list: a gate that is silently renamed or dropped fails nothing, it just stops checking. triage.py does not exist yet (lands in Task 3), but all three cases pass regardless: run_gates spawns python3 against the missing script, which still exits non-zero rather than crashing the harness, so the gates come back all-red as required by the fail-closed contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: bind the bot token to a step env var instead of reading secrets in if The secrets context is not reliably available in a step-level if expression. Binding it to the step's env and testing that works, and leaves the claim step live the moment P0.5 adds the secret, with no further workflow edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: split SKILL.md, add the artifact contract and the drift guard Rule bodies move to rules.md so the derived subset, not all of them, reaches each review. SKILL.md keeps the prose every review needs and gains what it lacked: each step now writes an artifact, so a skipped step and a performed one are no longer textually identical. The prompts quote SKILL.md verbatim and check_prompts.py asserts that relation. Both land together -- a guard shipped without the thing it guards is an assertion about nothing. Verified by introducing a one-character drift and confirming the guard goes red. Rule CONTENT is untouched; rewriting it from real ATOM review history is P1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: fix check_prompts.py ATOM naming, add missing Refutation section check_prompts.py still named aiter's artifacts (aiter-review-bot.yml, AITER_RUN_BUDGET, 'aiter-bot' login) after the port, so five of its checks would have skipped forever once Tasks 5-7 land their ATOM-named files -- two of them guard RUN_BUDGET < timeout-minutes and the self-trigger exclusion, so a silent skip there is a silent gap, not a formality. Renamed to atom-review-bot.yml / ATOM_RUN_BUDGET / rocm-atom-bot. rules.md was missing the '## Refutation' section that SKILL.md:323 and prompts/refuter.md both point to verbatim ('`rules.md` § Refutation has why.'). Ported aiter's Refutation section (not Tiering, which nothing references) and adapted its examples: aiter's own incidents and measured numbers are kept explicitly attributed to aiter, not invented as ATOM's, since ATOM has not yet run this skill long enough to have its own. Re-verified: check_prompts.py exit 0 (NO DRIFT), all 18 rule ids intact, drift-injection round trip still exit 1 then exit 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: port the seven gates, with a minimal ATOM deriver The gates are what make a skipped step visible, so they come first. The backbone tier table is ATOM's: Tier 1 here means 'runs on every forward pass' or 'base class for several model families', not 'import atom breaks' -- ATOM has no such import chain. derive() is deliberately minimal for P0. The full family axis is P1, built from real ATOM review history. It still never returns empty: an empty rules.txt means Step 1b did not run, and the ledger gate fails closed on it. Each gate is tested both ways. A gate tested only on the green path is a gate nobody has seen say no. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: assert the real empty-WORK-dir property, not card's job test_empty_work_dir_fails_closed demanded every gate go red on an empty WORK dir, which makes the card gate responsible for detecting "no work happened" -- not its job. card's scope is card-vs-verdicts consistency; an empty card with an empty verdicts.txt is genuinely nothing for it to catch. Reassert what each layer actually owns: answers/diagnostic/corefiles/ledger (by name) must each reject their own empty artifact, and _gates.py run as a whole against an empty WORK dir must exit non-zero -- the property the pipeline depends on, now asserted directly instead of approximated per-gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: port fetch.sh, dropping the three aiter-only forensics FlyDSL bounds, AOT pairing and struct ABI answer questions ATOM does not have. Everything else stays: the guard, sibling, symbol, twin, test-quality, CI-coverage and perf-claim collectors all describe defects ATOM ships too. Auto-validation defaults off because validate-atom-pr does not exist yet. A missing validator must read as an environment gap on the card, never as a finding against the PR and never as a failed review. The same applies to the D9 index-width scanner and its scale/schema inputs (also under validate-atom-pr/): they now degrade to a NOT RUN note instead of aborting the whole script before rules.txt is even derived -- verified empirically, since the hard-exit behavior inherited from aiter otherwise kills every review in P0. Once validate-atom-pr/ lands in P2 these checks go back to being hard requirements, unchanged from upstream. Also fixes triage.py's missing executable bit (100644 -> 100755, matching aiter's mode), without which fetch.sh cannot invoke it directly. Verified against two real merged ATOM PRs (#2705, #2697): every required artifact lands non-empty, and both git worktrees fetch.sh creates are cleaned up after each run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: move policy into version control The failure-class owner map and the trigger allowlist are policy: identical on every machine, and changing them should leave a trace. They become files next to the skill rather than repo variables, so reassigning who gets paged is a reviewable commit instead of a settings edit nobody sees. ATOM has one owner today and all five classes point at them. The classes still earn their keep -- they put the cause in the notification. BOT_NAME and BOT_REPO are dropped outright; GITHUB_REPOSITORY already says. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: fix fetch.sh's path taxonomy to ATOM's own tree TEST_PREFIXES and RUNTIME_PREFIXES still named aiter's directories (op_tests/, csrc/, hsa/, aiter/, gradlib/), so every ATOM test under tests/ fell through the unclassified-means-runtime fallback and was counted as runtime surface -- a tests-only PR's card carried a spurious "Validation (deterministic): NOT RUN" line instead of N/A. TEST_PREFIXES is now ("tests/",) and RUNTIME_PREFIXES is ("atom/",) -- verified against the actual repo layout (tests/ is the only test root, with its own conftest.py; atom/ is the runtime package; no csrc/hsa/gradlib anywhere). atom/ is named explicitly even though the fallback already covers it: the constant should state ATOM's own runtime surface, not another project's, and the explicit entry is a free backstop against a future STATIC rule accidentally shadowing part of atom/. Left scripts/, tools/, recipes/ unclassified on purpose -- genuinely ambiguous, and the fallback already defaults them to the safe side. Also regeneralised the fallback comment, which cited aiter/configs/*.csv and aiter/jit/** as examples; it now describes the same reasoning without asserting facts about a repository this skill no longer reviews. Verified by running classify() against real paths (not by reading) and by re-running fetch.sh against a real merged tests-only ATOM PR (#2265): the validation line now reads "not applicable: no runtime surface changed", not NOT RUN. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: port the orchestrator and its guards run_one.sh carries the failure taxonomy: every exit, including a signal kill, leaves a classified status, so a dead review names someone instead of dying quietly. selftest.sh holds it to that. Budget is 120 minutes inside a 150 minute job, up from aiter's 100/120, because validate will serve a model twice in the same run (P2). The GLM deep probe stays conditional on ANTHROPIC_BASE_URL: unset while we are on the gateway, live the moment this moves to box308's local GLM. Also fixes check_prompts.py's check_fail_classes, which still read _notify.py's pre-Task-5 `CLASSES` dict; Task 5 renamed it to `OWNERS` when policy moved into owners.txt. That check was silently skipping (missing run_one.sh) until this change made both files present, at which point it would have failed loudly on an attribute that no longer exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: port the three hosted-infra fallback test harnesses Task 5 ported watchdog.py, queue_gate.py and lost_review.py but not their _test.py companions -- selftest.sh calls all three, and until now they were the three reds hiding behind a real guard failure. These are the tests for the fallback paths that exist precisely so a review that dies on the self-hosted box still reports to somebody: a stuck queue, a dead runner, and a review that vanished mid-flight. Ported verbatim from the pinned aiter source, with the same renames the rest of this port has used (ROCm/aiter -> ROCm/ATOM, aiter-bot -> rocm-atom-bot, AITER_BOT_TOKEN -> ATOM_BOT_TOKEN in the entry_point fixtures, which would otherwise have exercised the wrong environment variable without ever failing for it). Also corrected file mode to 644, matching every other .py in this skill dir -- the three harnesses came in as 755 from the initial git-show, the same class of mode mismatch Task 4 found in triage.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: add the atom-bot trigger workflow and its own selftest lane The trigger workflow keeps aiter's four-job shape, and the reasons in its comments are kept with it: the gate job exists so an unauthorized comment never starts a job on the shared box, and the watchdog and report jobs run on hosted infra precisely so they are alive when the box is not. Three ATOM changes: the runner label is atom-bot-24 so ATOM jobs are never claimed by aiter's runner, the job cap rises to 150 minutes to sit above the 120-minute run budget, and the owner repo-variables are gone now that the map is a file in the repo. The selftest lane is ubuntu-only and gates every skill change without competing for ATOM's CI runners. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: fix check_owner_fallbacks for owners.txt's shape Task 7 landing the workflow activated this check for the first time, and it crashed: it still read _notify.CLASSES, a tuple shape (repo_var, owner, desc) that Task 5's move to owners.txt replaced with OWNERS (owner, desc). Rewrote it to compare watchdog.py/lost_review.py/queue_gate.py's hardcoded fallback owners against owners.txt directly, and dropped the half that asserted a workflow-injected OWNER_OVERRIDE var, since Task 7 deliberately removed that injection. The OWNER_OVERRIDE env read stays in the three scripts as a harmless, currently-unpopulated override channel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: add preflight and runner setup for srdc-24 RUNNER-SETUP.md leads with the thing that is easy to get wrong by copying aiter: that box serves its own GLM on all eight GPUs, and reproducing the arrangement here would leave none for validation. Here the model is reached over the network and the local GPUs stay free. The runner installs on /mnt/md0 because /mnt/raid0 is full, and as a systemd service with Restart=always because a hand-started runner dies on reboot and reports nothing: the job just sits queued, looking like a review in progress. This box adds a second way to lose it that aiter's never had: a runner started inside an agent container dies with the container, so registration and the systemd unit must be done from the host, not from here. preflight.sh's mode-detection comment also claimed aiter's provisioning choice (direct-agent mode) as this box's own; fixed to say this box runs tunnel mode via claude-glm, and left ATOM_REVIEW_AGENT unset in the documented .env so the check doesn't flip into the wrong branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: Task 9 verification sweep + P0 handoff doc P0 delivers the review chain end to end: fetch -> worker -> refuter -> seven gates -> card.md, all verifiable locally. It deliberately defers publishing (claim reaction, _publish.py --post, real PR comments) to P0.5, which needs a bot account that doesn't exist yet. Ran the full static verification sweep (pytest, selftest.sh, check_prompts.py, actionlint, preflight.sh) -- all five matched the controller's expected results exactly: 9/9 pytest, 83/0 selftest, 10/10 check_prompts with zero drift, clean actionlint, and preflight's 20 green / 1 red / 1 warning (exit 1) where the one red is the systemd-service check, correctly red because no runner can be registered or installed from inside this container. Did not attempt the brief's Step 1 (a live run_one.sh acceptance run against a real ATOM PR): it launches a headless sub-agent with --dangerously-skip-permissions, which this environment's auto-mode classifier blocks as "Create Unsafe Agents" -- not a false positive, the same constraint run_one.sh's own header documents. That run, and the token/wall-clock figure it would produce, is left to the user on srdc-24 in a plain terminal -- P0-HANDOFF.md spells out the exact invocation, how to pick a PR, what success looks like, how to read the card, what a red gate means, and the remaining runner-registration steps that need the user's own GitHub credentials. Updated the design spec (S5.1b, S9) to state plainly that the gateway-quota token/time figure is still unmeasured and why, rather than inventing a number in its place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: F1/F2 stop citest/kerneltest from manufacturing false evidence citest and kerneltest derive their all-clear from aiter's tree shape (.github/scripts/split_tests.sh + op_tests/, and KERNEL_DIRS's aiter paths). Neither exists in ATOM, so both silently produced false or dead-green output: citest named op_tests paths ATOM doesn't have, and kerneltest's untested-kernel filter can never match anything here, so it printed an affirmative "no new kernel file without a collectible test" on every PR. Both now check tree-shape applicability first and print an explicit NOT APPLICABLE line naming why, instead of a false or vacuous claim. Also corrects render_untested_kernel's HK6 citation, which named ATOM's test-reference-dtype-promotion rule for an unrelated finding. Also closes F3: UNREACHABLE_BY_DESIGN now lists the 21 rule ids rules.md carries that derive() can never emit (B3 B4 C3 D1b D2 D5 D6 D7 D8 D9 F1 G1 G1b P1 P2 P3 P4 P5 HK3 HK6 HK7), so `triage.py mapping` surfaces the gap instead of reporting none. MAPPING.md regenerated to match. SKILL.md's Step 3 now states $WORK/rules.txt is the authoritative rule set and its own inline rule references are pre-pipeline prose pending P1; rules.md's "Performance Evidence (always check)" header corrected to say P1-P5 are not derived today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: F5 add eleventh check closing _publish.py's permissions gap _publish.py falls back to ~/.git-credentials (a human PAT with write access) and posts over curl, so GITHUB_TOKEN scoping does not stop it from posting -- only the missing --post flag does, where every other path has two points of closure. Add check_publish_needs_post_flag: it fails if atom-review-bot.yml ever grants issues: write or pull-requests: write anywhere while _publish.py is invoked without --post, so the dependency bites instead of merely being visible. Proven by temporarily adding pull-requests: write to a job (check goes red, names the problem) and reverting (green again); see final-fix-report.md for both exit codes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: F6 surface the HTTP error body when a hosted-infra post fails queue_gate.py, watchdog.py and lost_review.py each logged only ::warning::could not post ... : <status> on a failed comment post. With no write grant every post 403s, so "your review never got a runner" and "this request was not queued" existed only as a bare status code in a job log. _notify.py already prints the failure detail; these three now match it. All three share watchdog.py's GitHub class, so the fix is there: GitHub._request now captures the HTTPError response body into self.last_error_detail, and the three warning lines append it when present. comment()'s return contract is unchanged (still a bare status int) so transport_checks' `status == 201` assertion and all three scripts' test fakes, which override _post directly and never touch _request, are unaffected -- the capture only activates against the real transport. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: F4 correct token-scope claim, F7 name the empty-card weakness, deferred minors F4: RUNNER-SETUP.md claimed ATOM_BOT_TOKEN is used "only in the claim and publish steps -- never in the review step", but it is bound at five places in atom-review-bot.yml including inside the review job. The safety-relevant fact still holds -- run_one.sh's own review step carries no such env and never exports one -- so the sentence now says that precisely instead. F7: P0-HANDOFF.md section 4 now names that a 0-finding card skips the refuter (run_one.sh writes NONE AVAILABLE itself) and a comment-only refutations.txt still clears its gate, so an empty card is the cheapest path through the pipeline and should be read the way an unremarked human approval would be. Inherited from aiter, not a port defect; not a gate fix. Four deferred minors: - fetch.sh: drop bin/ from STATIC_PREFIXES (a future runtime bin/ would be silently classified static); keep .cursor/. - run_one.sh:68: comment no longer names aiter's honglie as ATOM's backend owner; points at owners.txt instead. - watchdog.py/lost_review.py/queue_gate.py docstrings: state that nothing populates OWNER_OVERRIDE since the workflow injection was removed. - lost_review.py: restore aiter's reasoning for why the report job pages the flow owner rather than the runner owner (a triage job, not necessarily a box problem), from aiter c1c482e00's aiter-review-bot.yml. Includes the final-fix-report.md verification sweep for this wave. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: fix stale TIER backbone list, add directory-prefix matching triage.py's TIER tuple carried atom/models/deepseek_v4.py, which became the package atom/models/deepseek_v4/ on origin/main and can therefore never match a diff path again -- a corefiles-gate entry that silently stopped demanding any Step 4 assessment. TIER's matching was also exact-equality (`p in TIER`), so even naming a directory would never have worked. Measured with `git log --since=2026-04-27 --name-only` two levels deep over origin/main's atom/ tree, several hot paths were entirely absent from TIER: atom/model_ops/attentions (396 commits, only aiter_mla.py covered), atom/models/deepseek_v41 (165), atom/model_ops/v4_kernels (138), and the deepseek_v4 package itself (115) -- all above or comparable to existing Tier 2 entries. This is the root cause of merged ATOM PR #2683 (a deepseek_v41 mono/ fix) deriving only the generic-code fallback. Fixes: - Add _tier_hit(), matching an exact file or a "/"-suffixed directory prefix; TIER1/TIER2 are now defined once near the top of the file and reused by both derive()'s family axis and the corefiles gate's core_files_in(), removing the drift risk of two independent lists. - Drop the dead deepseek_v4.py entry; add atom/model_ops/attentions/, atom/model_ops/v4_kernels/, atom/models/deepseek_v4/ and atom/models/deepseek_v41/ as Tier 2 directory entries. attentions/ and v4_kernels/ are clean Q3 fits (ops shared across deepseek_v4, deepseek_v41, qwen4_exp); deepseek_v4/ and deepseek_v41/ are carried at Tier 2 on the same high-churn/high-blast-radius basis the pre-existing deepseek_v4.py row already used, not a strict shared-op fit -- documented inline as a softer call for P1 to revisit. - Deliberately do NOT add atom/plugin/sglang or atom/plugin/vllm to TIER despite being hotter (328, 323 commits) than most Tier 2 entries: SKILL.md's own Q4 already classifies a plugin bridge file as Tier 3, and derive()'s plugin-bridge family already fires on atom/plugin/ -- adding them to TIER would double-fire the same reason under two family names. - Update SKILL.md's Step 4 backbone table and surrounding prose to match. Verified: pytest (9 passed), selftest.sh (83 green/0 red), check_prompts.py (11 checks, 0 drift), triage.py mapping diffs clean against MAPPING.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: add D10/F2/A4/G2/H1/H2 rules, strengthen A1/G1/A4/P4 Six new rule ids wired end-to-end (body in rules.md, derive() trigger in triage.py, ALL_RULES, regenerated MAPPING.md): D10 (sentinel collides with valid data), F2 (unbounded per-request tracking set), A4 (distributed handshake/coordination invariant changes on one side only), G2 (CPU/GPU sync added to the decode loop, borrowed from vLLM/TensorRT-LLM), H1/H2 (new "Batch Object Invariants" family, ScheduledBatch mutated in place / batch-consumer purity, borrowed from sglang's schedule-batch rules). A4 folds in a third mining pass's two additional citations (ATOM#2000, Revert ATOM#2315) alongside the original ATOM#1994, making visible that one was caught in review and one needed a full revert. Strengthened without new triggers (per the mining reports' "strengthens existing X", not new rules): A1 (+ATOM#1091, +ATOM#2607, cross-plugin- backend sibling-scan note), G1 (+ATOM#2654 D2H-direction race, opposite of the existing ATOM#1369 H2D citation, + record_stream-vs-wait_stream criterion), P4 (+PP/DP/DCP/EP dimensions beyond TP). G1/G1b remain in UNREACHABLE_BY_DESIGN -- confirmed via fetch.sh on ATOM#1369 that no derive() trigger exists for them today; not changed here, reported rather than silently left inconsistent. Verified: pytest (9 passed), selftest.sh (83/0), check_prompts.py (0 drift), triage.py mapping regeneration (empty diff), expand on all 6 new ids (6 rule bodies, no MISSING-RULE-TEXT). Triggers demonstrated firing on real merged PRs: D10->#2012, F2+A4->#2146, A4->#1994, G2->#930, H2->#1414. H1 has no demonstrated firing on any real ATOM PR or commit found in history -- reported as unfinished rather than claimed working. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * review-pr: port scan_index_width.py so D9 becomes live D9 (int32 overflow in index arithmetic) was ATOM's best-evidenced rule and dead three ways: not in ALL_RULES, parked in UNREACHABLE_BY_DESIGN, and its backing scanner lived under validate-atom-pr/, which does not exist, so fetch.sh wrote NOT APPLICABLE forever. Port aiter's scan_index_width.py (AST pass, no name lists) verbatim into review-pr/ -- it feeds Step 1 review evidence, it is not a validation-pipeline executor, so it does not belong under a validate-atom-pr/ stub. Rewire fetch.sh's D9 block (previously gated on validate-atom-pr/'s existence) to find the scanner at its new path and run it unconditionally; SCALE/SCHEMA stay genuinely P2-gated (validate-atom-pr/'s own artifacts) and degrade to a note instead of a hard failure. Keep the NOT RUN degradation path for the case where the scanner itself is genuinely missing. Verified against ATOM's three known index-width overflow fixes (PR #2565 / 3795338967f, PR #2211 / 1580bf1af6a, PR #1460 / 7986573b10c): all three scan clean post-fix (0 candidates), because the fix line itself (a `.to(tl.int64)` widening) carries no multiply for the diff-scoped scanner to flag -- this confirms the scanner parses ATOM's Triton and does not false-positive on correct code, not that it re-examines the (unchanged) downstream multiply line. Also constructed a minimal synthetic unfixed case (`token_id * stride`, both runtime params, no widening) and confirmed it fires (1 candidate) and goes clean once `token_id` is widened to int64 -- proving the scanner also catches the defect shape, not just the fixed state. D9 stays in UNREACHABLE_BY_DESIGN -- correctly, as "scanner-backed, not derived", which is aiter's own design for this rule. The point of this change is that the scanner now exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: give G1/G1b a derive() trigger G1 carries two real citations pointing in opposite directions (#1369, H2D/compute-side; f08cd7f70/#2654, D2H-side) and could never fire -- confirmed by re-running fetch.sh on ATOM#1369, which derived rules=5/25 with G1 absent. A rule with real citations that can never fire is the exact disease this project keeps finding. Add a `stream-lifetime` family: added lines that create/hand off a stream or make a cross-stream lifetime promise explicit (cuda.Stream(, .record_stream(, .wait_stream(, .set_stream(, .to(..., non_blocking=True)) surface both G1 (sync-ordering) and G1b (the record_stream-vs-wait_stream criterion) together. Move both out of UNREACHABLE_BY_DESIGN into ALL_RULES. Demonstrated firing on ATOM#2654 (f08cd7f70): rules=7/27, [stream-lifetime] G1 G1b. Still does not fire on #1369 itself -- that PR's diff has no stream API call at all, just a bare tensor handoff -- reported honestly in rules.md rather than re-tuned to force a match on the PR that motivated the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: land the Triton rule family (T1/T3/T5/T6/T8), graded by evidence Wires the vendored-but-unreachable triton_families()/hardcoded_launch_knob() into derive(), gated on `@triton.jit` in added lines or a path under atom/model_ops/{fla_ops,v4_kernels,attentions,deepseek_v41,qwen4_exp, minimax_m3,kimi_k3} or atom/plugin/vllm. Not a flat port -- each rule is graded by its own evidence: - T1 (masked load, aiter's duplicate B2 dropped to avoid an id collision with ATOM's own unrelated B2): regression guard on currently-correct code (fla_ops/chunk_delta_h.py), not a known ATOM defect. Demonstrated on a synthetic unmasked tl.load/tl.store (real ATOM loads in the directories checked are already masked). - T3/T8 (launch knobs): carry real ATOM evidence -- v4_kernels/paged_decode.py's _kernel_config() heuristic (lines 105-133, call sites 712/787-788/846-847) for T3, and the hardcoded num_warps=4 at line 896 for T8. T3 demonstrated firing on ATOM PR #1788 (kimi_k3 attention_residual.py); T8 demonstrated on a synthetic hardcoded num_warps= literal (real PR sampled used a variable, not a literal). - T5/T6: carried from aiter with NO ATOM citation, labeled as such in rules.md same as aiter marks its own uncited rules. T5 demonstrated firing alongside T3 on PR #1788 (acc accumulator pattern); T6's trigger wiring is exercised by the derive() gate but not demonstrated on a real PR here. - T2 (mask with no other=) and T4 (AMD launch knobs hardcoded ACROSS archs) are NOT ported: T2 was never checked against ATOM; T4 was checked and found zero hits for waves_per_eu/matrix_instr_nonkdim/kpack in the sampled directories -- "not found in sampled directories", not "absent from ATOM". family_mapping()'s derive()-body-only scan is extended to also pull in triton_families/hardcoded_launch_knob's own bodies when derive() calls them by name, so MAPPING.md's generator keeps working now that these helpers are reachable. MAPPING.md regenerated: 25 families, 32 rule ids. Verified: pytest 9/9, selftest.sh 83/0, check_prompts.py 0 drift, triage.py mapping diffs clean against MAPPING.md, triage.py expand emits all 5 new rule bodies (T1 T3 T5 T6 T8). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: add E4 "a production kernel/dispatch belongs in aiter, not ATOM" New rule E4, sourced from the closed-unmerged-PR channel (not the inline- comment corpus the rest of rules.md draws from) -- the strongest cluster found there: ATOM#2465, #1104, #2510, #1593 were all closed, not merged, after clearing local correctness/perf review, once a maintainer or author noticed the same specialization already lived (or was decided to belong) in aiter. ATOM#987 is the same boundary one layer up (CI, not kernel). Trigger scoped to new-file-only for the kernel half, per the task's explicit constraint: ATOM has 116 existing Triton kernel files, so firing on every Triton edit would be noise disconnected from this defect. Fires on a NEW file that is self-contained (no `from aiter import`/`aiter.` call) and declares a kernel (`@triton.jit` or `__global__`). A second, narrower trigger (not new-file-scoped, since #2510 itself added its switch to existing files) covers a new `ATOM_*` env-var dispatching between ATOM-side kernel variants under atom/model_ops/ or atom/models/. Demonstrated firing on ATOM#1104 (the causal_conv1d.py PR itself, via fetch.sh): `[production-kernel-belongs-in-aiter] E4`, alongside the item-3 Triton family (T1/T3/T5/T6/T8) firing on the same real PR as a bonus confirmation of that wiring. MAPPING.md regenerated: 26 families, 33 rule ids. Verified: pytest 9/9, selftest.sh 83/0, check_prompts.py 0 drift, triage.py mapping clean against MAPPING.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: add D8's first ATOM-native citation (ATOM#2371) D8 (kernel call missing contiguous check) had evidence from elsewhere but no ATOM instance of its own. ATOM#2371 (reviewer xytpai, found via the closed-PR channel -- the PR itself was closed as superseded by #2431, not closed for this defect) reproduced a non-contiguous src_token passing the wrapper's device/shape/dtype validation and producing 1,056 mismatched output bytes with no exception. Text-only change to rules.md; D8 remains in UNREACHABLE_BY_DESIGN (no derive() trigger change in this commit). Verified: pytest 9/9, selftest.sh 83/0, check_prompts.py 0 drift, triage.py mapping unchanged (text-only edit). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: state plainly that H1's trigger has never fired on a real PR Strengthens H1's existing "no ATOM PR citation yet" line, which conflated "no evidence for the defect" with "the wiring has never been exercised by a real diff" -- two different claims. Re-searched for this increment (gh search prs/issues for ScheduledBatch-mutation language; grepped the tree for batch.<field>.append(/.extend(/.add_(/.fill_(/prev_batch.<field> +=/-= outside tests) and found nothing: one incidental hit (atom/plugin/register.py:1084, forward_batch.positions.add_(1)) turned out to be SGLang's own ForwardBatch, not ATOM's ScheduledBatch, and isn't even matched by H1's batch/prev_batch name scope. H1 stays wired (body + trigger + ALL_RULES, not moved to UNREACHABLE_BY_DESIGN) since the trigger is reachable by design -- just explicitly flagged as never demonstrated, rather than left to look proven by association with the rules that are. Verified: pytest 9/9, selftest.sh 83/0, check_prompts.py 0 drift, triage.py mapping unchanged (text-only edit), triage.py expand on all 34 landed rule ids emits 34/34 bodies with no MISSING-RULE-TEXT. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: fix T8's trigger to match type-annotated hardcoded launch knobs T8 (triton-launch-knob-hardcoded) fired 0/14 times on Triton-gated PRs in the P1 sample despite 48 ATOM files carrying a hardcoded num_warps/num_stages. LAUNCH_KNOB required `name = digit` with nothing in between, so a very common ATOM shape -- a type-annotated wrapper default like `num_warps: int = 4,` (atom/model_ops/triton_fused_sigmoid_mul_quant.py, PR #2534) -- never matched. Allow an optional `: type` between the knob name and `=`. Also exclude `tl.range(..., num_stages=N)`: that sets inner-loop software- pipelining depth, not a launch configuration, and firing on it would be over-triggering on every pipelined loop rather than a second instance of the defect T8 targets. Verified against ATOM#2534 via fetch.sh: T8 now derives on the real diff (triton-launch-knob-hardcoded, alongside T3/T6). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: drop T5 (triton-accum-prec), no ATOM citation and 0/14 fires T5 ("accumulator precision loses tail-of-sum bits") was carried from aiter with no ATOM instance from the start, and P1's 26-PR distribution measurement found it fired 0/14 times across all Triton-gated PRs -- it cannot answer "which real ATOM change would have gone unreviewed without this rule". Re-searched ATOM's git log and merged-PR history for an fp16/bf16-accumulator precision-loss fix or review comment before removing it; found none (the closest hits -- e.g. PR #2764's FP32-accumulator register-pressure tuning -- are a different defect, not tail-of-sum precision loss). Removed from ALL_RULES, derive()/triton_families(), and rules.md's body (a removal note explaining the grading replaces the rule body, matching this branch's convention of saying why a rule isn't there rather than leaving a silent gap). Not moved to UNREACHABLE_BY_DESIGN -- that list is for evidence that arrives by another route, not a parking spot for a rule that never earned its place. MAPPING.md regenerated via `triage.py mapping`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: record the 20000-line fetch cap as a known P0 gap ATOM#2688 (240 files) couldn't be fetched during P1's distribution measurement -- its diff exceeds GitHub's API diff size cap and fetch.sh fails closed, as designed. Record it under P0-HANDOFF.md's "deliberately left undone" so the largest, arguably highest-review-value PRs being unreachable is a known limitation, not a surprise. Not fixing it here -- real fix needs a local-diff fetch path, out of scope for this pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: tighten Step 4's core_files.txt wording to stop non-backbone files from getting a line A worker hit MALFORMED on core_files.txt: Step 4 said what to write when NO Tier 1/2 file is touched (a NONE line), but never said what to do with a non-backbone file (e.g. a new test) touched alongside one that IS backbone -- the worker guessed and wrote it a per-file N/A line in a format the gate doesn't accept. Now Step 4 says explicitly: a line only for backbone files, nothing at all for the rest, in either case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: prefix the card's verdict line so it can't be miscounted as a finding The refutations/independent gates (and _apply_refutation.py) count any card line starting with 🔴/⚠️/📝 as a reported finding -- correctly, that's the rule. But the template rendered the verdict as a bare `[✅ LGTM | ⚠️ NEEDS WORK | 🔴 BLOCK]` line, so a real card's `⚠️ NEEDS WORK` verdict line itself got counted as a finding. Measured on the first real run (ATOM#2705): card had 1 real finding, the gates counted 2, and _apply_refutation.py independently reported the same miscount -- any card with a non-clean verdict had one more "finding" than a refuter could ever defend, so it could never pass. Following aiter's shape, the verdict line now reads `Review: [...]`, matching the label-prefix convention aiter already uses to keep its own verdict line out of the finding count. The gate's counting rule is untouched -- this is a template fix, not a gate fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: add the mandatory Validation (deterministic) line, per design spec 6.6 SKILL.md's card template had no Validation or Perf line at all (0 occurrences, against 11 in aiter's SKILL.md) -- never implemented, not lost, since ATOM's pre-pipeline original also had 0. The design spec (docs/superpowers/specs/2026-10-23-atom-review-bot- design.md §6.6) requires it: a card that says nothing about validation reads as clearance. Add the line to the template with its five states adapted from aiter's SKILL.md, and add a card-gate check (triage.py's new _check_validation_line, wired into audit_card) so a card missing it -- or merging N/A and NOT RUN, which are different facts -- is caught rather than accepted. P0 ships no validator, so in practice this reads NOT RUN or N/A for now; the richer states are for when validate (P2) lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: sync worker.md's quoted template to the Review/Validation fix, add regression test check_prompts.py polices prompts/worker.md's quoted copy of SKILL.md's Step 8 output rules for drift; the verdict-prefix and Validation-line changes left it stale. Re-copy the block verbatim so check_prompts.py goes green again. Also add the regression test the 22 green commits never had: a fixture card carrying a non-✅ verdict and exactly one real finding (trimmed from the real ATOM#2705 run) that asserts the refutations and independent gates go green, that the verdict line is never counted as a finding, and that the card gate both accepts a clean card with the Validation line and rejects one missing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: fix validation_requirement.json's test-target detection for ATOM's tree Step 1's validation triage carried aiter's op_tests/ convention verbatim into is_candidate_target(), so `candidates` was always empty on ATOM (whose tests live flat under tests/, e.g. tests/plugin/*.py) -- a runtime PR that adds a test always fell through to "ships no test target" regardless of what it actually added. Confirmed on ATOM#2705: it adds tests/plugin/test_minimax_m3_mono_refusal.py, and validation_requirement.json still claimed no test target existed. Fixed to match ATOM's tests/ layout (excluding tests/benchmarks/, same reasoning as aiter's op_benchmarks/ exclusion); re-running Step 1's generator against the real PR now correctly selects it as target with target_basis "the one test target this PR touches". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: explain STALE + give recovery when a PR's base is downstream of its own merge merge-target/ is checked out at $BASE_SHA, the base branch's CURRENT tip -- for an already-merged PR that is routinely downstream of the PR's own merge commit, since main kept moving after the merge landed. `apply --check` then fails for a reason that has nothing to do with drift: the diff's changes are already part of $BASE_SHA's history, so re-applying them conflicts by construction. The old STALE message read this as "needs a rebase" regardless of cause, and gave no recovery path. Confirmed on ATOM#2705: merged via e3a00a9281, base tip ebd5f84f3a6af6a99d23f85e5c1107b238d1cdaf is downstream of that merge commit (git merge-base --is-ancestor confirms it), and applies.txt said STALE/needs-rebase with no explanation. Fixed by fetching mergeCommit.oid via `gh pr view --json ...,state,mergedAt,mergeCommit`, testing ancestry with `git merge-base --is-ancestor $MERGE_SHA $BASE_SHA`, and writing a distinct STALE message for this case: names the true cause (already merged, not drift), warns not to read merge-target/ as this PR's actual base, and gives the recovery command `git show $MERGE_SHA^:<path>` to read pre-PR state. The genuine-rebase-needed STALE message for open PRs is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * run_one.sh: don't let the merge-target recheck clobber fetch.sh's double-application diagnosis run_one.sh's applies.txt recheck exists to correct WHICH TREE git apply --check ran against (fetch.sh tests PROJECT_ROOT's current checkout but reports it as the merge target), not to replace the explanation. But fetch.sh can now diagnose a specific, much more useful STALE case -- the PR is already merged and BASE_SHA is downstream of its own merge commit, so re-applying the diff conflicts by construction, not from drift -- and the recheck unconditionally overwrote that with the generic "needs a rebase" text. Measured on ATOM#2705: fetch.sh produced the double-application diagnosis and recovery instructions; run_one.sh threw them away. Fix: when the recheck also comes back STALE, keep fetch.sh's double-application message if that's what it already wrote (ancestry between the merge commit and BASE_SHA doesn't depend on which worktree ran the apply --check), and only fall back to the generic rebase text when fetch.sh's own verdict was generic too. Added a selftest guard exercising the recheck's shipping code directly (STALE/STALE, STALE/generic, and the unaffected APPLIES-overwrites-regardless case), since neither this bug nor the recheck's existing behavior had ever been pinned. Verified end-to-end: ran run_one.sh against PR #2705 with the stub agent and confirmed applies.txt carries the double-application text (merge SHA e3a00a9281..., recovery via `git show <sha>^:<path>`) rather than the generic rebase message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * run_one.sh: GLM health probe false-negatives against an Anthropic-style endpoint The probe fires on any ANTHROPIC_BASE_URL and only ever posts the OpenAI-compatible shape to /v1/chat/completions. ANTHROPIC_BASE_URL promises nothing about that shape -- measured in this environment it points at an Anthropic-style gateway serving /v1/messages instead, so the probe got 404 and aborted the whole review with "GLM backend unavailable", class glm, paging the model owner for a backend that is in fact healthy (claude-glm resolves and works fine). Fix: try the OpenAI shape first (unchanged default path for an on-box GLM, forwarding any gateway headers from ANTHROPIC_CUSTOM_HEADERS), and only if that doesn't come back 200, also try the Anthropic shape (/v1/messages, x-api-key, anthropic-version) before concluding anything. This keeps the deep real-inference check the probe exists for (aiter's point: a shallow /health stays green while inference is wedged) working regardless of which shape the backend actually speaks, instead of guessing one shape or letting the env dictate without asking it. A 404 on BOTH shapes is specifically "the endpoint answered HTTP and recognizes neither path we tried" -- a probe-shape/gateway-routing problem, not evidence the backend is dead -- so that case warns and lets the run proceed instead of aborting. Anything else non-200 (connection failure/timeout, a 5xx on either attempt) still aborts hard: those are the real "wedged or down" signals this probe exists to catch, and are left untouched. Failure class: if a wrong-shape probe were escalated at all it would be `env` (a box/gateway configuration fact), never `glm` (paging the model owner for something that isn't the model's fault) -- but per the fix above it doesn't warrant failing the run at all, so no class is actually filed for this path; `glm` stays reserved for the signals that really do mean the backend is unhealthy. Added a selftest guard driving the shipping probe code against a stubbed curl across both shapes x several code combinations (200 first-try, 404-then-200, 404/404 warn, 000/404 and 500/500 still abort), since this behavior had never been pinned. Verified end-to-end: ran run_one.sh with ANTHROPIC_BASE_URL set to this environment's real Anthropic-style endpoint. The first (OpenAI-shape) probe still 404s as before, but the second (Anthropic-shape) probe, with ANTHROPIC_CUSTOM_HEADERS forwarded, returns 200 -- a real, successful deep-inference probe -- and the run proceeds past the health gate instead of aborting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(review-pr): route kv_transfer, mesh, and entrypoints/openai to rule families Four of ATOM's hottest directories derived no rule family at all: a PR under atom/kv_transfer/offload, atom/kv_transfer/disaggregation, atom/mesh/src or atom/entrypoints/openai could only reach an existing rule if its diff happened to match that rule's content regex -- the path itself routed nowhere, even though real defects (and existing rule citations) already live in exactly these directories. - kv-transfer: pure path trigger (atom/kv_transfer/offload|disaggregation, mooncake) -> A4, D10, F2. Replaces the narrower, content-gated pd-handshake family -- all three rules already carry citations anchored in these two directories (D10: ATOM#2305, F2: ATOM#2146, A4: ATOM#2000 / the ATOM#2315 revert). - mesh-routing: atom/mesh/src/ -> new rule A5 ("routing/dispatch accounting state not threaded through every request path"), anchored on ATOM#2351 (WorkerLoadGuard built only for non-streaming requests) and ATOM#1886 (DP-affinity rank preserved on non-streaming but not streaming paths) -- same shape, two independent PRs. - openai-entrypoint: atom/entrypoints/openai/ -> A1 (ATOM#1803, the same content="" fix repeated across three sibling SSE chunk-builder call sites) and B6 (ATOM#2041, a rejected request's terminal path never touches _seq_id_to_callback, hanging the SSE stream forever). rules.md gains A5; ALL_RULES and MAPPING.md updated to match (25->27 families, 32->33 rule ids). Verified fetch.sh derives each new family on a real PR: mesh-routing on #2351, openai-entrypoint on #2041, kv-transfer on #2167. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: lint _notify.py rather than exempt it Unlike the other vendored gate infrastructure, _notify.py's owner-resolution was substantially rewritten for ATOM (Task 5 replaced aiter's CLASSES dict + repo-variable override with a file-backed owners.txt lookup) -- roughly half the file's lines differ from upstream, not just renames. That is squarely 'substantially rewritten for ATOM', so it gets the normal lint treatment instead of joining the vendored exemption: a context manager for the status read (ruff SIM115) and black's formatting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * pyproject: exempt review-pr's vendored gate files from black/ruff review-pr is vendored from ROCm/aiter at pinned commit c1c482e00 and carries aiter's code style, which trips both lint gates (206 ruff errors, 16 files black would reformat). Follow the precedent already set for the FlyDSL trees: exempt by file, not by directory, so the exemption tracks what is actually vendored rather than everything that happens to live under review-pr/. Exempted (vendored, kept close to upstream so the port stays diffable against `git -C aiter show c1c482e00:<path>` -- that diff is what let reviewers confirm vendored logic changed only where intended, across ten deliberate ATOM couplings): triage.py, _lib.py, _gates.py, _collect.py, _apply_refutation.py, _publish.py, queue_gate.py (+ _test), watchdog.py (+ _test), lost_review.py (+ _test), check_prompts.py, scan_index_width.py. Deliberately left linted (and fixed to pass): - _notify.py: substantially rewritten for ATOM, not carried over with renames -- Task 5 replaced aiter's CLASSES dict + repo-variable override with a file-backed owners.txt lookup, ~55% of the file's lines differ from upstream. Fixed in the prior commit (ruff SIM115, black formatting). - review-pr/tests/: written for ATOM on this branch, not vendored. Fixed three PLW1510s (subprocess.run without explicit check=) and one unused noqa, then black-formatted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: resolve rules.md's 14 zero-evidence rules A3/D1/D6 get real ATOM citations (commit 4c7067735 unmerged/no-PR, open PR#2357, merged PR#119 respectively) -- A3's new citation confirmed firing via fetch.sh on PR#2535 (ep-tp-shapes -> A3 B2). B1/C2/D4/E3 keep their borrowed aiter citations but are now honestly labeled "no ATOM instance found despite search", same convention as H1/G2. D2, D7, and the whole P1-P5 Performance Evidence family are dropped: zero citations and no sound preventive trigger to fall back on. P1-P5 were additionally in UNREACHABLE_BY_DESIGN while their own section header claimed "always check" -- doubly weak. triage.py's perf_claims()/test_quality() evidence-gathering logic is untouched; only their prose stopped citing the now-dead P1/P2/P3 ids. Cascades: UNREACHABLE_BY_DESIGN trimmed in triage.py, MAPPING.md regenerated (not hand-edited), SKILL.md's Step 3 checklist/Step 6 table/Output-rules examples updated, prompts/worker.md's verbatim- synced block updated in lockstep. Verification: 13 pytest, selftest 93/0, check_prompts 0 drift, triage.py mapping diff empty, ruff clean -- all match the pre-task baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: enforce Validation (deterministic) entitlement in the card gate PR#2705's archived card (reports/PR-2705/card.md) is the real fixture: the worker ran a test itself with the Bash tool and labelled the result `Validation (deterministic): PASS`. All seven gates passed it because the card gate checked the line was present, never that it was entitled -- `(deterministic)` means a head-matched validation_report.json that ships its own reproducer, and no such file exists for a model's own test run. triage.py's audit_card()/_check_validation_line() now reject any Validation (deterministic) verdict other than N/A/NOT RUN unless $WORK/validation_report.json exists -- the file fetch.sh writes only after its identity gate confirms repo.head/base/patch_sha256 match the exact PR. The honest-work path the worker had no label for gets one: Validation (advisory), mirroring aiter SKILL.md's Perf (deterministic) / Perf (advisory) split -- one run, this box, unverified, never upgraded. SKILL.md's card contract now states the entitlement rule and the two validation tiers where the reviewing model reads it (still <=500 lines). worker.md's verbatim-quoted copy of that section is kept in sync (check_prompts.py enforces this). Note: triage.py's own card-gate edit for this landed via a concurrent commit (e98f3c3ae) that swept up this session's working-tree change alongside its own rules.md work, rather than as a commit from this session -- confined strictly to the card-gate function as instructed, derive()/ALL_RULES untouched. Regression tests added in test_card_template_regression.py cover: the archived PR-2705 card now fails the gate (UNENTITLED-VALIDATION), a bare (deterministic) claim with no report fails the same way, one backed by a validation_report.json passes, the honest N/A/NOT RUN forms pass, and the new Validation (advisory) tier passes without a report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: inline the PR-2705 entitlement fixture; keep CI green test_archived_pr_2705_card_is_rejected_by_the_card_gate read reports/PR-2705/card.md off disk. reports/ is untracked runtime output _collect.py writes after a real review, never added to the repo -- it exists on a machine that has run a review and nowhere else, so the test passed locally and failed on every fresh CI checkout. Fix: inline a trimmed copy of that card's text (same Validation (deterministic): PASS-with-no-report shape) as ARCHIVED_PR_2705_CARD_TEXT, with a comment pointing at the real archived card and explaining why it isn't read from disk. The test still exercises the exact same card-gate path and still asserts UNENTITLED-VALIDATION. Not fixed by skipping-when-absent (a skip reads as a pass) and not fixed by committing reports/ (it's generated output, not source). No other test in the file touched untracked or machine-local paths. Also ran Black on the file -- .claude/skills/review-pr/tests/ is written for ATOM on this branch, not vendored, so it isn't covered by the aiter exemption and must stay formatted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Document bare `atom-bot review` as the trigger, not an @-mention GitHub notifies on @handle regardless of what we document, and atom-bot the real user) is dormant but still real, so the trigger phrase itself must never be written with @. The bot's account stays rocm-atom-bot everywhere it identifies itself (PAT, dedup marker, self-comment filter, preflight identity check) -- only the documented, typed trigger changes. `atom-bot review` is a substring of `@atom-bot review` and `@rocm-atom-bot review`, so typing the habitual @-form still works and still (harmlessly, since the account is dormant) notifies that account -- that's GitHub's doing the moment the comment is sent, not this workflow's, and is not worth fighting. Because the bare phrase lacks an @-mention's natural anchoring, it can otherwise appear inside ordinary prose, a quoted log, or a code block. Tightened the gate's regex to require the (optionally @-prefixed) phrase open the line (after trimming leading whitespace and skipping quoted `>` lines, as before) rather than matching anywhere via \b. Added test_trigger_regex.py, which extracts the live pattern straight out of the workflow YAML and exercises it against the bare phrase, both @-forms, a quoted trigger, and a sentence that merely mentions the bot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Retarget runner label to machine-neutral atom-bot; document srdc-24/box308 as two phases The runner label atom-bot-24 named a specific box and would mislead once the runner moves; atom-bot works unchanged across both phases and stays distinct from aiter's box308 label. RUNNER-SETUP.md is rewritten around a srdc-24 (testing, now) vs box308 (production, later) comparison as its spine, since neither box replaces the other: systemd availability, disk path, GPU availability, and model endpoint all invert between them. Documents the live GPU constraint this creates on box308 (its GLM-5.3 holds all 8 GPUs, so P2's validate-atom-pr has none there -- points at design spec section 5.3 for candidate resolutions without picking one), the two-runner coexistence with aiter (actions.runner.ROCm-ATOM.* vs ROCm-aiter.*, confirmed no collision with preflight.sh's *ATOM* unit pattern), and the CPU/disk contention between the two runners during concurrent reviews. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * P0.5: enable publishing now that rocm-atom-bot and ATOM_BOT_TOKEN exist _publish.py is invoked with --post so the review card actually lands on the PR (rocm-atom-bot's own token, not GITHUB_TOKEN -- the workflow still grants no issues/pull-requests write, so check_publish_needs_post_flag stays satisfied by "no write anywhere" rather than by the --post/write pairing branch). The claim-👀 step's condition was already written to test the secret rather than assume its absence, so it needs no change -- only its comment, which no longer describes a step that's dormant. Verified rocm-atom-bot's actual access before relying on it: not a collaborator on ROCm/ATOM (push: false), token scoped to public_repo only -- sufficient on a public repo to comment and react without write permission. Updated P0-HANDOFF.md's "what P0 delivers" section, which asserted P0 posts nothing; that stopped being true once P0.5 lands. Added a regression test pinning that _publish.py's main() calls do_post only on a POST decision with --post, never otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(review-pr): make the publish post-flag test hermetic main() re-reads the card to grade severity after a successful post, so the test depended on reports/PR-2705/card.md -- an untracked local run artifact. Present locally, absent on a fresh CI checkout: green here, red there. Point REPORTS at tmp_path and supply the card. Second instance of this same defect class (cf. the card-template regression test); both now hermetic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: land D11 (FlyDSL unbounded descriptor) and teach flydsl_bounds ATOM's own mono/ surface ATOM authors its own FlyDSL kernels under atom/mono/ and the model-specific atom/models/{deepseek_v41,minimax_m3}/mono/ trees -- a surface five months old, discovered mid-P1, never mined. atom/mono/device/ops.py:rsrc(addr, nbytes=None) omits nbytes the same way aiter's max_size=True default does, yielding an unbounded descriptor; PR#2683 (fixed e1e8cb034) shows the real shape: a candidate-row offset in index_score.py could land outside its token's row with nothing to catch it, clamped in Python rather than bounding the descriptor. Landed as D11, scoped to ragged/row-addressed extents -- the miner's other ~18 rsrc() call sites in atom/mono/ are fixed-extent and safe by inspection, so the rule does not fire on all of them. Separately, triage.py's flydsl_bounds() forensic gated on aiter's own conventions only: a "flydsl" substring in the path, and aiter's three buffer- call names. Neither matches ATOM's mono/ trees or its rsrc() wrapper, so the forensic has never once run against the one place a real merged bug of its exact shape lives. Broadened the path test (ATOM_FLYDSL_PATH) and added rsrc to FLYDSL_BUF_CALLS with its own bound-arg handling, keeping aiter's names and paths working. Restored fetch.sh's flydslbounds step (dropped at the initial port as aiter-only) now that it has something ATOM-shaped to look at. Also derive() now routes atom/mono/ and atom/models/*/mono/ paths to a new flydsl-unbounded-descriptor family (D11) -- narrow by design, ~1.4% of commits over the trailing six months touch that surface. Noted in rules.md: atom/model_ops/v4_kernels/paged_decode_gluon.py (Gluon, PR#2377) is a clean negative -- one file, no history, no rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: land B7 (parallel-linear missing prefix=) and I1 (stateful cache outside content-addressed pool), add B2's MoRI citation B7 (Silent Bypass family): a ColumnParallelLinear/RowParallelLinear/ ReplicatedLinear/MergedColumnParallelLinear/QKVParallelLinear built with no prefix= queries get_layer_quant_config("") and silently falls through to the global quant spec instead of the checkpoint's own exclude-layer config. Anchored on ATOM#755 (954a66f8c, Qwen3-Next MTP's fc) and ATOM's own .claude/commands/add-model.md, which documents the exact mechanism. Wired in derive() via a new ctor_missing_kwarg() helper, scoped to atom/models/ and atom/model_ops/ so the LinearBase/ColumnParallelLinear class definitions themselves do not trigger it. I1 (new section, Stateful Cache Invariants): a per-request state buffer (SWA ring, GDN/mamba recurrent state) kept outside the content-addressed paged KV cache breaks prefix-cache safety -- a cross-request hit reuses the shared KV blocks but reads stale per-request state. Best-evidenced rule in the set: issue #1417, fix PR #1423 (081045f6a), follow-up PR #1573, and an independent statement of the same invariant in docs/scheduling_kv_cache_guide.md (~line 320). Wired as a pure path trigger on today's locus of the invariant (atom/model_engine/{state_cache,state_pool,state_runtime, page_unit_checkpoint,block_manager}.py, the GDN/DSv4 attention backends, and both plugin bridges #1423/#1573 touched). B2 gets a second, ATOM-native citation: PR#2320 (7acd75755) removes a comment reading "`internode` must come from a real topology probe. It used to be inferred from `world_size <= 8`" -- the same size-proxy-instead-of-probe shape B2 already describes, in MoE all2all routing rather than attention. MAPPING.md regenerated via `triage.py mapping`. Tests added in tests/test_new_rule_derive.py cover both derive() triggers (positive and negative) plus mapping/expand round-tripping. Verified before writing: `git show` on 954a66f8c, 081045f6a, 7acd75755 and `gh api .../pulls/{755,1423,1573,2320}` for merge state; issue #1417 and docs/scheduling_kv_cache_guide.md read directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(review-pr): state that ATOM needs its own runner registration, not aiter's A repository-scoped runner serves only the repo it was registered to, so 'share aiter's runner' is not reachable without re-registering aiter's at the org level. The setup section already described two units; make the reason explicit so the handoff does not start by trying to reuse aiter's token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * review-pr: drop the run's git worktrees on failure paths too Cleanup lived only on the success path, so every timed-out or failed run leaked its two worktrees -- both the checkouts and the registrations in the repo, which nothing ever prunes. On a long-lived self-hosted runner shared with aiter's bot that grows without bound; this box had 140 leaked dirs / 2.7G / 7 stale registrations from testing alone. Keeping WORK itself on failure stays deliberate: diagnosing the withheld review on PR#2695 needed its verdicts.txt. But the worktrees are ~99.5% of the size and are reproducible, while the text artifacts are ~200K and are not -- so the traps now drop the checkouts and keep the evidence. Verified: a real worktree created under a fake WORK is removed and the registration pruned (8 -> 7), with verdicts.txt left in place. Divergence from aiter c1c482e00, which cleans only on success and leaks the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(review-pr): add the admin handoff, and correct two stale facts ADMIN-HANDOFF.md collects the four things that need admin rights on ROCm/ATOM -- runner registration token, the ATOM_BOT_TOKEN secret, two optional repo vars, and an interaction-limit check -- so the ask is one page rather than a conversation. Two corrections it surfaced: - RUNNER-SETUP.md said rocm-atom-bot 'doesn't exist yet'; it was registered today. - The two repo *variables* the gate and watchdog read were documented nowhere; they …
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Latest follow-up commit:
42be727b48ffd5f7d6de1ca40b1424d2e07dff8d.Tune MiniMax-M3 prefill index scoring for gfx1250 (MI455, wave32).
SCORE_NUM_STAGES.causal masking, score computation, top-k ordering and sparse-table emission
are unchanged.
This update is incremental to PR head
be613ac1d56778f8dd7f8594410b741a2905aa6e,which used 8 warps, 1 stage for gfx1250 FP8. It does not reapply the already
present stages=1 change; it selects four warps based on direct comparison.
Reducing stages from the original 3 to 1 reduces resource pressure. However,
four warps are not equivalent to eight simply because gfx1250 uses wave32.
For the matched FP8 case, Triton reports 204 registers / 16,384 B LDS for
the previous
(8,1)configuration and 288 registers / 16,384 B LDS for(4,1), with zero reported spills in both. We therefore do not attributethis incremental gain to fewer registers or claim an occupancy mechanism
without hardware-counter evidence.
New validation (current launch policy)
ruff check: passed.black --check --target-version py312: passed.pytest -q tests/test_minimax_m3_index_topk.py: 172 passed, 1 skippedon each of two separate MI455 GPUs.
selectors. GPU numerical tests cover BF16 and E4M3FN; no claim of GPU
numerical coverage for every FP8 format.
the previous PR configuration: 1/4 heads, contiguous/strided Q, ragged
requests including a zero-query request, partial pages, all-zero Q ties,
and the matched prefill shape.
entry and sparse context length match exactly. No tolerance was relaxed.
_prefill_score_launch_configis the only changedexecutable function in
index_topk.py.New same-machine performance results
Environment: f01 GPU0, MI455/gfx1250, 256 CUs; image
rocm/fw-bringup-mlperf:gfx1250-atom-20261023; PyTorch2.11.0+rocm10.1.0a20260811-bkc.20260820, HIP7.16.26315, Triton3.9.0.No GPU clock/power changes.
Method: synthetic tensors with the listed prefill geometry, BF16 Q,
FP8 E4M3FN or BF16 index cache, head dimension128, shuffled physical pages,
page size128, top-k16, forced init0/local1. Identical data, query tile and
chunk geometry across candidates. Score-only warm-cache GPU timing: 20 calls
per HIP Graph, nine samples per round, three interleaved rounds per arm
(base/candidate/candidate/base/base/candidate), pooled median of 27 samples.
JIT and profiler collection are outside timing.
Before = previous PR
(8,1)for FP8,(8,3)for BF16.After =
(4,1)for FP8; BF16 configuration unchanged.Speedup = before/after. Complete paired results, including effectively unchanged
or slightly slower measurements:
For the issue #341 matched TP4-local shape (
Q=2517, prefix=102784,context=105301, one index head per rank), FP8 score improves
56.997 -> 40.268 us (1.415x, 29.4% lower latency) relative to this PR's
previous configuration. With four local heads, the same geometry improves
199.975 -> 132.290 us (1.512x); this is a different workload, not the TP4
per-rank result.
The matched score + top-k + sparse-table emission chain improves
69.550 -> 52.729 us (1.319x) in separate paired Graph timing.
Top-k is not modified. Isolated kernel and full-chain timings are not assumed
additive.
Trace verification
Separately captured operator traces confirm:
(20,1,64); block size changes from 256 to 128 threads(8 -> 4 wave32 warps).
_index_block_score_kerneland the same_topk_index_packed_kernel.11.056 -> 11.015 us. Profiler timings are supporting evidence, not the
source of the Graph speedup table.
Numerical and end-to-end limitations
TP4 whole-model trace, serving result, or GSM8K run.
make_tiled_tdm_atomAPI neededby the pinned M3 attention path. Whole-model startup failed before serving;
no inference/accuracy result is claimed for this follow-up.
sampled scores beyond
rtol=atol=3e-5, maximum absolute difference2.8801e-4, against FP32/FP64 dot. Triton/PyTorch Q-to-FP8 casts matched;sampled top-k matched. That pre-existing discrepancy is not claimed fixed.
This launch-only change preserves the scorer's output exactly in regression
cases; it does not silently change arithmetic to match a different oracle.
results are effectively unchanged. The historical 114.93 us issue figure
is not the baseline of this new speedup calculation.
Historical model-level validation (previous 8-warp revision)
The following evidence was posted by the original PR author before this
four-warp follow-up. It is retained for provenance and does not validate
the current four-warp policy at model level.
Image:
rocm/fw-bringup-mlperf:gfx1250-atom-20261016.Original test result: 133 passed, 1 skipped. BF16/FP8, 1/4-head short and
matched-shape comparisons were exact.
Matched MiniMax-M3 prefill (
tok=2517,ctx=105301, TP4, FP8 KV):5.850 ms score kernel time per step.
across 57 sparse layers.
launches).
Do not combine/multiply the historical model-level speedup with the new
isolated-kernel speedup.
Scope and reproduction
Only the existing launch policy and
tests/test_minimax_m3_index_topk.pyare modified. No RMSNorm changes, benchmark-specific kernel, docs files,
new environment variables or changes to model configuration are introduced.
Raw timing samples, comparison drivers, compiler assembly and trace are retained
in the experiment workspace; the implementation does not detect benchmarking.
MI355 / gfx950 compatibility verification
Rechecked against follow-up commit
42be727b48ffd5f7d6de1ca40b1424d2e07dff8d.The non-gfx1250 branch returns before the new FP8 policy is considered:
SCORE_NUM_STAGESremains 3. Consequently:Verification performed:
test_prefill_score_launch_config: 10 passed, 163 deselected.These are architecture-policy tests with the reported architecture mocked;
they do not execute on an MI355 GPU.
(4, 3)results for gfx950 and gfx942 across FP16, BF16, E4M3FN, E4M3FNUZ,
E5M2 and E5M2FNUZ dtype selectors.
be613ac) confirmed that_prefill_score_launch_configis the only executable function changed bythis follow-up. The score kernel, causal masking, top-k selection,
sparse-table emission, chunk geometry and decode implementation are unchanged.
Conclusion: the gfx950 launch policy and computation path are unchanged.
This is a source/policy compatibility check, not a new MI355 GPU execution,
performance, or whole-model accuracy validation. No zero-regression performance
claim on MI355 is made without an on-device run.