diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 005a06f87..3b787b12a 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -94,6 +94,7 @@ If docs and live code disagree, verify the code/tests and report the drift. | --- | --- | | [model-addition-checklist.md](references/checks/model-addition-checklist.md) | A model, architecture, loader, processor, registry, or stage config is added. | | [perf-verification.md](references/checks/perf-verification.md) | The PR makes a latency, throughput, memory, or quality claim. | +| [config-provenance.md](references/checks/config-provenance.md) | Changed or newly load-bearing behavior reads config, CLI, environment, legacy, or default values. | | [test-quality-evaluation.md](references/checks/test-quality-evaluation.md) | Tests change, are absent for risky code, or may not exercise production behavior. | | [tests-docs-checklist.md](references/checks/tests-docs-checklist.md) | Coverage, CI markers, examples, user docs, or PR evidence need review. | | [verification.md](references/checks/verification.md) | Hardware, a server, or a runnable affected path is available for active verification. | @@ -152,13 +153,19 @@ contract they protect or use only the applicable evidence checks. Apply every category in [general-checks.md](references/process/general-checks.md) before lower-priority comments. -For each changed value or behavior, trace: +For each changed or newly load-bearing value or behavior, trace: ```text public ingress -> validation/defaulting -> producer -> transformations -> stage/worker/connector boundary -> final consumer -> terminal cleanup ``` +Treat unchanged context as in scope when the diff makes it authoritative for a +new consumer. When config, CLI, environment, legacy, or default sources can +control that value, apply +[config-provenance.md](references/checks/config-provenance.md) and prove the +documented source at the final sink with distinct sentinel values. + Cover every applicable offline/online, streaming/non-streaming, sync/async, feature-on/off, topology, and compatibility path. Search bounded callers and sibling implementations rather than assuming the changed hunk is the only path. diff --git a/.claude/skills/review-pr/references/checks/config-provenance.md b/.claude/skills/review-pr/references/checks/config-provenance.md new file mode 100644 index 000000000..f7af64414 --- /dev/null +++ b/.claude/skills/review-pr/references/checks/config-provenance.md @@ -0,0 +1,49 @@ +# Configuration Provenance + +Read this reference when changed or newly load-bearing behavior reads config, +CLI, environment, legacy, alias, or default values. Always use it for endpoint, +host, port, address, connector metadata, and `*_extra_config` consumers, even +when the schema or documentation is unchanged. + +## Trace the source of truth + +For each semantic value, record one compact ledger: + +```text +documented input -> parsed field -> competing sources and precedence + -> runtime projection/transport -> final consumer +``` + +- Search the exact field, environment variable, default literal, and aliases in + code, tests, docs, examples, and deployment files. +- Treat unchanged producers and context lines as review scope when the diff + starts relying on them or makes their output authoritative. +- Resolve the intended precedence from active branch-local docs, enforced + code/tests, and compatibility policy. Report a documented input that is + silently ignored or reinterpreted at a live sink. +- Check every supported construction path and feature-off behavior without + inventing parity for unsupported paths. + +## Require discriminating evidence + +Give every competing source a distinct value so the test reveals which source +wins. Prefer non-default sentinels; for example: + +```text +documented config port = 25301 +environment/default port = 8998 +expected final endpoint = http://10.0.0.8:25301 +``` + +Assert the final URL, connection target, runtime argument, or other live sink, +not only an intermediate object. A test that changes the environment value to +the expected configured value proves formatting, not configuration propagation. +For a bug fix, show the discriminating test fails on the frozen base and passes +on the reviewed head when the environment permits execution. + +## Finding bar + +A supported non-default value reaching the wrong endpoint, device, topology, or +runtime behavior is a correctness or compatibility finding. A missing +discriminating test alone is a validation gap unless repository policy requires +that regression coverage for the change. diff --git a/.claude/skills/review-pr/references/checks/test-quality-evaluation.md b/.claude/skills/review-pr/references/checks/test-quality-evaluation.md index fb1427f01..ae139d715 100644 --- a/.claude/skills/review-pr/references/checks/test-quality-evaluation.md +++ b/.claude/skills/review-pr/references/checks/test-quality-evaluation.md @@ -16,6 +16,9 @@ were reverted or broken. Check: instead of the changed behavior being mocked away; - fakes preserve relevant types, MRO, shapes, devices, async behavior, and lifecycle transitions; +- configuration tests give competing config, CLI, environment, legacy, and + default sources distinct non-default values and assert the final consumer; + setting only the source read by the implementation does not prove precedence; - seeds, ordering, timing, synchronization, external services, and numeric tolerances are deterministic or explicitly controlled; - normal, invalid, boundary, feature-off, failure/cancellation, and regression diff --git a/.claude/skills/review-pr/references/process/general-checks.md b/.claude/skills/review-pr/references/process/general-checks.md index 88dfd405e..ff29c7262 100644 --- a/.claude/skills/review-pr/references/process/general-checks.md +++ b/.claude/skills/review-pr/references/process/general-checks.md @@ -15,6 +15,9 @@ and [design documents](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/ - Trace each changed value through public ingress, validation/defaulting, producer, transformations, process or device boundaries, final consumer, and terminal cleanup. +- Treat unchanged code as in scope when the diff makes its value newly + load-bearing for a changed consumer; validate that value's origin and contract + rather than reviewing only added lines. - Check every applicable sync/async, offline/online, streaming/non-streaming, feature-off, topology, and compatibility path without demanding unsupported modes merely for symmetry. @@ -123,7 +126,9 @@ distinct live caller, invariant, or compatibility need. ## Finding bar -Anchor each finding to a changed `path:line`; name the trigger or call path, -current behavior, impact, and smallest fix direction. Treat pending CI, missing -hardware, or unsupported measurements as validation gaps unless the repository -contract makes that evidence a merge requirement. +Anchor each finding to the exact `path:line` that causes the behavior; it may be +unchanged when the diff makes it newly load-bearing, in which case name the +changed call path that brings it into scope. Include the trigger, impact, and +smallest fix direction. Treat pending CI, missing hardware, or unsupported +measurements as validation gaps unless the repository contract makes that +evidence a merge requirement. diff --git a/.claude/skills/review-pr/references/process/review-execution.md b/.claude/skills/review-pr/references/process/review-execution.md index f06f27933..8de201aa3 100644 --- a/.claude/skills/review-pr/references/process/review-execution.md +++ b/.claude/skills/review-pr/references/process/review-execution.md @@ -166,6 +166,10 @@ index patch, worktree patch, and untracked-content manifest. Any mismatch makes the review stale; discard the affected evidence and restart. Prefer one root-cause comment to several symptom comments. +When the root cause is an unchanged line made newly load-bearing by the diff, +cite it in the consolidated review body instead of moving the comment to an +unrelated changed line merely to make it inline. + Local presentation is the default. Only explicit authorization permits GitHub posting. When authorized, publish one consolidated final review after validation; do not post preliminary or incremental comments. Submit a review diff --git a/.claude/skills/review-pr/references/process/review-routing.md b/.claude/skills/review-pr/references/process/review-routing.md index 10768d329..977059d4f 100644 --- a/.claude/skills/review-pr/references/process/review-routing.md +++ b/.claude/skills/review-pr/references/process/review-routing.md @@ -50,6 +50,7 @@ compatibility changes. | --- | --- | --- | | New or expanded model, loader, processor, registry, or stage config | [model-addition-checklist.md](../checks/model-addition-checklist.md) | [`add-tts-model`](../../../add-tts-model/SKILL.md) or [`add-diffusion-model`](../../../add-diffusion-model/SKILL.md) | | Latency, throughput, memory, scaling, precision, or quality claim | [perf-verification.md](../checks/perf-verification.md) | [`diffusion-perf-opt`](../../../diffusion-perf-opt/SKILL.md) for diffusion | +| Config/CLI/environment/default precedence; endpoint, host, port, address, connector metadata, or `*_extra_config` | [config-provenance.md](../checks/config-provenance.md) | None | | Tests changed, absent for risky behavior, or test-only | [test-quality-evaluation.md](../checks/test-quality-evaluation.md) | [`vllm-omni-test`](../../../vllm-omni-test/SKILL.md) | | CI, examples, docs, public behavior, or contributor evidence | [tests-docs-checklist.md](../checks/tests-docs-checklist.md) | None | | Suitable hardware/server and runnable affected path | [verification.md](../checks/verification.md) | None |