diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 0b19b2b37..5907e13f3 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -123,10 +123,11 @@ it changes again, report the churn and wait for a stable target. For a trusted PR head, materialize the pinned head in an isolated detached worktree. A worktree freezes identity but is not a security sandbox. Treat fork -heads as untrusted unless the user and environment policy explicitly establish -otherwise: execute them only in a disposable, secret-free sandbox with restricted -filesystem, network, and resources; without one, use static SHA-addressed reads -and CI evidence only. For a local review, freeze the committed, index, worktree, +heads as untrusted until the user and environment policy explicitly establish +trust: execute their code only after that trust is recorded for this host, +with the trusted SHA noted in the review state and secrets kept out of the +execution scope; without recorded trust, use static SHA-addressed reads and CI +evidence only. For a local review, freeze the committed, index, worktree, and NUL-safe in-scope untracked contents. Follow [review-execution.md](references/process/review-execution.md) for trust gates, state fingerprints, and byte-for-byte staleness checks. @@ -196,7 +197,7 @@ style preferences as simplification findings. Before each validation group, verify the frozen SHA plus the tracked, index, untracked, and ignored-file fingerprint, or recreate a pristine snapshot. On a -trusted head or inside the required sandbox, run an import/version preflight, +head the user explicitly trusted for this host, run an import/version preflight, then the narrowest relevant tests and low-cost static checks. Bind every result to the head SHA, snapshot fingerprint, and environment fingerprint. Never run imports, tests, builds, hooks, or repo-configurable tooling from an untrusted diff --git a/.claude/skills/review-pr/references/checks/perf-verification.md b/.claude/skills/review-pr/references/checks/perf-verification.md index bbfe5a147..027e91589 100644 --- a/.claude/skills/review-pr/references/checks/perf-verification.md +++ b/.claude/skills/review-pr/references/checks/perf-verification.md @@ -22,6 +22,19 @@ Keep exact commands and report variability rather than a cherry-picked run. Use the repository's benchmark when it covers the claim; otherwise use the smallest reproducible workload that reaches the changed production path. +## Isolation per claim + +Isolate one claim per comparison. When a PR stacks several changes (a fused or +restructured load path, a distilled step schedule, a new cache), request one +column per claim at equal steps/workload instead of one blended base-vs-head +delta, and name the unchanged stage or metric that pins equivalence. + +When the changed path is runtime-switchable (feature flag or environment +variable), run every column on the same head and toggle the switch rather than +diffing against a base commit that cannot separate the stacked changes: +base, head with the switch off, head with the switch on, then head with the +switch on plus each further claim. + ## Evidence table | Dimension | Typical evidence | @@ -34,6 +47,100 @@ smallest reproducible workload that reaches the changed production path. Every optimization needs correctness or quality evidence. A faster result on a different workload, precision, seed, or topology is not a valid comparison. +## E2e stage-attribution table + +For an end-to-end latency claim, request the stage-attribution table the +repository's perf PRs use: + +- Rows carry the benchmark's own field names: total e2e, per-stage execution, + postprocess, response encoding, and a request-to-output roll-up. Add a stage + handoff/idle row when the pipeline spans stages — connector transfer and + scheduling gaps are invisible in per-stage latency deltas. +- Sanity rows pin comparability: inference steps, output frames/pixels/samples, + and generated tokens held identical across columns. +- Report P50 and P100 from the stated warmup and measured counts. One + population per number: never mix cold-start and warm samples into one value. +- Name the baseline and candidate commit SHAs, and close with an isolation + statement naming the metric that stayed unchanged ("X remained effectively + unchanged, isolating the gain to Y"). + +When the serving path caches, captures, or regenerates at runtime (CUDA-graph +bucket growth, cache rebuilds, recompilation), medians hide the resulting tail. +Also request the event count and the maximum observed stall. + +Component-level tables pair with, not replace, the e2e table: per-op reference +vs candidate timings (with the numerical-equality result per op) plus one +complete-pipeline check under the production shapes. + +## Proportionality + +When a PR adds a model and explicitly labels its numbers as smoke observations +rather than claims, do not demand the attribution machinery. Ask only for +evidence protecting the model's stated operating contract — for a +realtime/duplex model that is sustained cadence over a realistic session +(enough ticks to leave warmup), with any mid-session capture/regeneration +event and its stall reported, since a cadence that collapses mid-session is a +functional failure, not a perf claim. + +## Realtime/duplex world models + +Request the operating-contract evidence as serving metrics, not claims: + +- Sustained per-tick latency over one realistic session: median, max, and the + argmax tick. +- Stall events with their causes. +- Peak memory at first versus last tick — session-owned state must not grow + per tick. +- Control latency from a submitted action to the tick that applies it. +- Realtime-versus-offline parity under the same controls, using the + repository's video-similarity helpers. Parity plotted over tick index is the + drift-over-horizon curve and the honest measure of session lifetime; + distribution-level suites (FVD, VBench) belong to model evaluation, not the + serving integration. + +Watch the contract boundaries the model documents: if a control input is +tick-API-only, the parity pair must use a horizon both paths can run. + +## Speech-generation (TTS) models + +The streaming contract outranks the average — a synthesis that pauses +mid-stream is broken for realtime use regardless of its averages. Request: + +- The benchmark's own speech metrics over one long utterance (enough audio to + leave warmup, tens of seconds): `audio_ttfp` (time to first packet), + `audio_rtf` (real-time factor; above 1.0 cannot keep up), `e2el`, and + `ttft`/`tpot` where tokens precede audio. +- Mid-utterance stalls as the benchmark's continuity fields, not prose: + `audio_underrun` percentiles and the `audio_continuity_ok` rate + (`vllm_omni/benchmarks/metrics/metrics.py`), computed by + `vllm_omni/benchmarks/audio_continuity.py` — the companion to `audio_ttfp` + and `audio_rtf`. A synthesis that bursts chunks fast enough on average but + underruns mid-stream fails the streaming contract even when the averages + pass. +- Quality as a paired comparison against the reference implementation under + the same text, speaker prompt, and seed; report sample rate and frame + count, and assert output completeness — truncated or dropped trailing audio + is a known failure class, not a quality nuance. +- For realtime or duplex TTS: interruption (barge-in) latency and the + per-session memory trend across turns. + +## Omni models + +Apply the e2e stage-attribution table directly to the voice-to-voice path of +omni (speech-to-speech and multimodal AR) models: + +- TTFT for text output, `audio_ttfp` for audio output, and per-stage + ITL/TPOT cadence across the perception-understanding-generation chain. +- Cross-modal parity — the omni analogue of realtime-versus-offline parity: + the same semantic request through text and audio inputs, compared on output + content or timing. +- For duplex omni sessions: turn-taking latency, interruption handling, the + concurrent session count actually validated, and a per-turn memory trend — + session-owned KV state must not grow per turn. When either turn produces + audio, continuity is judged with the same `audio_underrun` and + `audio_continuity_ok` fields as TTS, since turn-taking latency alone does + not expose mid-utterance underruns. + ## Reviewer verification ladder 1. Run base/head A/B on suitable hardware when affordable. diff --git a/.claude/skills/review-pr/references/process/general-checks.md b/.claude/skills/review-pr/references/process/general-checks.md index 88dfd405e..da86eabc7 100644 --- a/.claude/skills/review-pr/references/process/general-checks.md +++ b/.claude/skills/review-pr/references/process/general-checks.md @@ -121,6 +121,19 @@ Search bounded callers and sibling implementations before reporting dead code, duplication, or a missed consumer. Keep new abstractions only when they have a distinct live caller, invariant, or compatibility need. +Verify absence and equality claims with whole-file evidence. A line window, a +grep for one symbol, or a commit message proves presence, never absence: to +assert that a file matches another version or lacks a change, diff or hash the +full files (for example `git diff : :` or the contents +API's size and SHA fields) before anchoring the finding. A verification +shortcut that would itself be wrong to run is not evidence. + +For a value written or finalized in more than one place, establish which site +executes first on each entry path — and whether the earlier site makes the +later ones unreachable — before naming where a fix belongs. Enumerating all +call sites correctly still misses the bug when a path the enumeration ranked +last actually finalizes first and clears the state the proposed fix checks. + ## Finding bar Anchor each finding to a changed `path:line`; name the trigger or call path, diff --git a/.claude/skills/review-pr/references/process/review-execution.md b/.claude/skills/review-pr/references/process/review-execution.md index 7c0305254..c78532e9d 100644 --- a/.claude/skills/review-pr/references/process/review-execution.md +++ b/.claude/skills/review-pr/references/process/review-execution.md @@ -39,13 +39,13 @@ isolated detached worktree. Run every source read, `rg` search, import, and test from that snapshot, not the caller's checkout. A detached worktree provides snapshot isolation, not a security boundary. -Treat a fork head as untrusted unless the user and environment policy explicitly -establish trust. Never run its imports, tests, builds, hooks, package setup, or -repo-configurable linters/plugins on the reviewer host. Execute them only in a -disposable sandbox or VM with no credentials or inherited secrets, no host agent -or service sockets, minimal read-only host mounts, disabled or explicitly -allowlisted network, resource/time limits, and destruction after the run. If -that boundary is unavailable, limit the review to the remote diff, SHA-addressed +Treat a fork head as untrusted until the user and environment policy +explicitly establish trust. Without recorded trust, never run its imports, +tests, builds, hooks, package setup, or repo-configurable linters/plugins on +the reviewer host. Once the user explicitly trusts this pinned commit for +this host, record the trusted SHA with the review state and execute with +credentials, agent sockets, and other secrets out of scope. Without recorded +trust, limit the review to the remote diff, SHA-addressed reads such as `git show :`, and existing CI evidence; report all executable validation as a gap.