fix(kwctl): support context-aware policies when using bench command#1877
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates kwctl bench to correctly support context-aware policies when replaying recorded host-capabilities sessions by rewinding the replay cursor before each benchmark iteration, and fixes --num-samples so it configures the sample count (instead of the resample count).
Changes:
- Add a replay reset handle for recorded host-capabilities sessions and reset it before each
validate_settings()/evaluate()bench iteration. - Fix
--num-sampleswiring so it updatesBenchmarkConfig::num_samples. - Add e2e coverage ensuring replay sessions are not exhausted during bench loops; extend proxy replay implementation with a shared cursor + reset handle and additional unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/kwctl/tests/e2e.rs | Adds an e2e test ensuring context-aware replay sessions are reset for every bench iteration. |
| crates/kwctl/src/command/bench.rs | Resets replay session cursor before each benchmarked evaluator call. |
| crates/kwctl/src/cli/bench.rs | Fixes --num-samples to set num_samples (not num_resamples). |
| crates/kwctl/src/callback_handler/proxy.rs | Implements replay cursor + reset handle; parses recorded requests once; updates replay logic and tests. |
| crates/kwctl/src/callback_handler/mod.rs | Exposes replay_reset_handle() through CallbackHandler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Allow `kwctl bench` to rewind a recorded host-capabilities session before every iteration, instead of exhausting it after the first evaluation. Also fix `--num-samples` being wired to the resample count. Signed-off-by: Flavio Castelli <fcastelli@suse.com> Assisted-by: Claude Sonnet 5.0
80cc79a to
8106fcc
Compare
|
I've resolved the copilot issues, squashed the commits and pushed them back |
Allow to run typos checks like done on the GH pipeline Signed-off-by: Flavio Castelli <fcastelli@suse.com>
Signed-off-by: Flavio Castelli <fcastelli@suse.com>
|
JFYI: one of copilot's reviews was a bit of an AI slop, but it led me down into a rabbit hole that produced something nice: wapc/wapc-rs#219 . |
The --doctests flag of cargo-llvm-cov is unstable and requires a nightly toolchain, while the repository pins stable via rust-toolchain.toml. This caused the doctest phase of `make coverage-rust` (and the CI coverage job) to fail with "the option `Z` is only accepted on the nightly compiler". The flag provides no value: the workspace has no runnable doctests, as all doc code blocks are marked `ignore` or use non-Rust fences (console, hcl, json). Also fix the Codecov upload path, which pointed to coverage/lcov.info instead of coverage/rust/lcov.info where the lcov report is written, so uploads were silently empty. Signed-off-by: Flavio Castelli <fcastelli@suse.com> Assisted-by: Claude Sonnet 5.0
viccuad
left a comment
There was a problem hiding this comment.
LGTM. Thanks for fixing the coverage job!
Allow
kwctl benchto rewind a recorded host-capabilities session before every iteration, instead of exhausting it after the first evaluation.Also fix
--num-samplesbeing wired to the resample count.Update: I also got a failure of the GH pipeline because I introduced a typo. I've extended the Makefile to have
typosandtypos-fixtargets