[Review Council — Claude] low · testing

_DELAY = 0.15 is fragile for the overlap-observation tests:

_DELAY = 0.15  # seconds; long enough for perf to start while warmup is pending

On a heavily-loaded CI runner, the entire warmup batch (5 requests) can plausibly round-trip within 150 ms before the perf phase issues, producing a false "no overlap" and flaking test_drain_false_overlap_observed / test_drain_false_max_concurrent_exceeds_single_phase_size.

Fix: use a controllable handler that explicitly blocks until the test signals release (e.g. via asyncio.Event) instead of a fixed sleep. If sticking with a sleep, increase to ≥1s.

[Review Council — Claude] low · testing

All warmup integration tests run with num_workers=1:

_MINIMAL_CLIENT = HTTPClientConfig(
    num_workers=1, warmup_connections=0, max_connections=10
)

None cover the multi-worker warmup path, where ZMQ ordering between worker processes matters and where salted prompts must be distinct across workers. Suggest adding at least one warmup test with num_workers >= 2 to exercise the multi-process distribution path the production code uses.

[Review Council — Claude] medium · bug

When the prompt is a multimodal list where ALL parts are non-text (e.g., all image_url), the loop finds no text part and falls through to return data with no warning:

if isinstance(prompt, list) and prompt:
    for i, part in enumerate(prompt):
        if isinstance(part, dict) and part.get("type") == "text":
            ...
            return {**data, "prompt": salted_parts}
return data  # unsupported prompt type — skip salting (silent!)

Unlike the input_tokens case (which emits a logger.warning), this fallback is completely silent. Users who enable salt=True for image-only multimodal datasets will not see any indication that KV-cache bypass is not functioning. Add a logger.warning consistent with the input_tokens path.

[Review Council — Claude] low · testing

sd.data = inner.data is redundant now that SaltedDataset.data is a property that delegates to self._inner.data. The assignment calls the property setter which sets self._inner.data = inner.data — a no-op since they reference the same list. This won't cause a test failure, but it misleads readers into thinking SaltedDataset.data is a plain attribute:

inner.data = ["raw string sample"]  # override with non-dict
sd = SaltedDataset(inner)
sd.data = inner.data  # redundant — sd.data property already returns inner.data

Remove the sd.data = inner.data line.

[Review Council — Claude] medium · bug

When the prompt is a multimodal list where ALL parts are non-text (e.g., all image_url), the loop finds no text part and falls through to return data without salting and without any warning:

if isinstance(prompt, list) and prompt:
    for i, part in enumerate(prompt):
        if isinstance(part, dict) and part.get("type") == "text":
            ...
            return {**data, "prompt": salted_parts}
return data  # unsupported prompt type — skip salting (silent!)

Unlike the input_tokens case (which emits logger.warning), this fallback is completely silent. Users who set salt=True for image-only datasets will see no indication that KV-cache bypass is not functioning. Add a logger.warning here, consistent with the input_tokens path.

[Review Council — Claude] low · testing

sd.data = inner.data is now redundant: SaltedDataset.data is a property that delegates to self._inner.data. The assignment calls the setter which sets self._inner.data = inner.data — a no-op since they already reference the same list.

inner.data = ["raw string sample"]  # override with non-dict
sd = SaltedDataset(inner)
sd.data = inner.data  # redundant — property already returns inner.data

Remove the sd.data = inner.data line.

[Review Council — Claude] medium · error-handling

The new docstring on _drain_inflight claims a safety guarantee that only holds in some configurations:

async def _drain_inflight(self, phase_issuer: PhaseIssuer) -> None:
    """Wait for all in-flight responses from this phase to complete.

    Bounded by the global experiment timeout: if the caller schedules a
    loop.call_later that calls stop(), stop() sets _drain_event, unblocking
    this wait without leaving it hung indefinitely."""
    ...
    self._drain_event.clear()
    await self._drain_event.wait()

The loop.call_later is only scheduled in execute.py:561 if ctx.rt_settings.max_duration_ms is not None. runtime_settings.from_config maps max_duration_ms == 0 to None, and the schema default is 0 — so the default offline/online CLI run has no global timer, and _drain_inflight retains its old unbounded await self._drain_event.wait(). A single dropped or hanging request will hang the benchmark forever.

The docstring suggests this case is solved; it isn't.

Suggested fixes:

• Make the docstring conditional ("if a global timeout is configured…") and add a note that the drain is otherwise unbounded, OR
• Add an intrinsic drain timeout (a fixed cap like 30–60s, or a multiple of observed completion latency) inside _drain_inflight itself so the safety property is a property of the function, not a contract on the caller.

[Review Council — Claude] low · testing

test_performance_sample_order_identical_with_and_without_warmup does not actually exercise the property it claims to test:

def make_rt():
    return RuntimeSettings(
        ...
        rng_sched=random.Random(99),
        rng_sample_index=random.Random(99),
        ...
    )

ctx_with = self._make_ctx(config_with, make_rt(), simple_dataset)
ctx_without = self._make_ctx(config_without, make_rt(), simple_dataset)

make_rt() is called twice and produces two separate RuntimeSettings instances with freshly-seeded Random(99) RNGs. _build_phases in the warmup path uses dataclass_replace which substitutes new Random(seed) instances on the warmup phase only — it never touches the perf phase's RNGs. So this test only verifies that _build_phases doesn't drain the perf RNG, which it never could (it only stores references). It does not verify the regression it claims to guard against: that executing the warmup phase leaves the perf-phase RNG untouched.

A tighter test would either:

1. Build phases from a single shared RuntimeSettings, then advance the warmup phase's rng_sample_index (e.g., draw N samples from create_sample_order(warmup_rt)), and assert that the perf-phase order matches a control with no warmup, OR
2. Drop this test as redundant with test_warmup_phase_uses_independent_rngs (which already asserts warmup_rt.rng_sched is not perf_rt.rng_sched). The current implementation gives a false sense of coverage for the actual race the docstring describes.

[Review Council — Claude] low · api-contract

RequestHandler = Callable[[web.Request], web.Response | Awaitable[web.Response]] doesn't allow None, but _dispatch (line 110) explicitly relies on None to fall through to the default echo handler. A user-provided handler that forgets to return therefore silently passes through default behavior instead of failing loudly — and the type alias misleads them about what's supported. Either widen the alias to ... | None, or assert non-None inside _dispatch so the error surfaces.

[Review Council — Claude] low · testing

test_warmup_uses_independent_rng_instances only checks is not identity. This passes even if both warmup RNGs were seeded with the same seed as perf — i.e. distinct objects but identical sequences, which would still cause warmup to consume the same state pattern as perf. Add an assertion comparing the first few drawn values, e.g. assert [warmup_rt.rng_sched.random() for _ in range(5)] != [perf_rt.rng_sched.random() for _ in range(5)], to actually catch the regression the test claims to guard.

[Review Council — Claude] low · testing

SaltedDataset.load_sample has a non-trivial branch at dataset.py:471 that warns when input_tokens is present without prompt, and a salt-the-prompt branch when both are present (which still leaves input_tokens unsalted — see existing comment on dataset.py:467). Neither path is exercised here. Add tests for (a) {"input_tokens": [...], "prompt": "..."} (salting still happens but input_tokens unchanged — silent miss for token-based adapters), and (b) {"input_tokens": [...]} only (warning fires, data returned unchanged).

[Review Council — Claude] low · api-contract

Settings.warmup: WarmupConfig has no cyclopts.Parameter annotation, so cyclopts auto-generates only dotted CLI flags like --settings.warmup.enabled=true, --settings.warmup.n-requests=.... Compare with runtime and load_pattern which expose ergonomic shorthands via field-level cyclopts.Parameter(alias=...). As-is, enabling warmup from the CLI is unwieldy. Add cyclopts.Parameter(alias="--warmup-...") aliases on the WarmupConfig fields, or document that warmup is YAML-only.

@arekay-nv Can you clarify the purpose and usage of this? Conceptually a salted dataset is just a normal dataset, but on load_sample, it prepends a randomly generated fixed length salt right? I'm wondering if adding a salt: bool = False argument to the base class .load_sample function is warranted. A lot of the body of this class is just remaps to the underlying dataset.

I like that approach. Updated. Thanks!