feat: add Pi provider and brainstorm benchmark variants#40
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new PiProvider adapter to support model routing via the pi CLI, updates default provider configurations, and implements experimental brainstorm iteration strategies (blind, score-only, own-reviews, full-visibility) behind a hidden CLI flag. The reviewer feedback focuses on code optimizations and robustness, specifically recommending direct comparison of ModelId instances to avoid string allocations during sorting, parsing numeric scores that may be returned as strings in JSON, and preserving the USERPROFILE environment variable on Windows to ensure cross-platform compatibility for the pi CLI.
| history_text.push_str("No peer evaluations were available for this answer.\n"); | ||
| } else { | ||
| let mut reviews = entry.reviews.clone(); | ||
| reviews.sort_by(|a, b| a.evaluator.to_string().cmp(&b.evaluator.to_string())); |
There was a problem hiding this comment.
Since ModelId implements Ord, you can compare the evaluators directly instead of converting them to String via to_string(). This avoids unnecessary string allocations during sorting.
| reviews.sort_by(|a, b| a.evaluator.to_string().cmp(&b.evaluator.to_string())); | |
| reviews.sort_by(|a, b| a.evaluator.cmp(&b.evaluator)); |
| for round in history { | ||
| let _ = writeln!(history_text, "<round number=\"{}\">", round.round); | ||
| let mut proposals: Vec<(&ModelId, &String)> = round.proposals.iter().collect(); | ||
| proposals.sort_by(|(a, _), (b, _)| a.to_string().cmp(&b.to_string())); |
There was a problem hiding this comment.
Since ModelId implements Ord, you can compare the model IDs directly instead of converting them to String via to_string(). This avoids unnecessary string allocations during sorting.
| proposals.sort_by(|(a, _), (b, _)| a.to_string().cmp(&b.to_string())); | |
| proposals.sort_by(|(a, _), (b, _)| a.cmp(b)); |
| history_text.push_str("<evaluations>\n"); | ||
| let mut reviews = round.evaluations.get(model_id).cloned().unwrap_or_default(); | ||
| reviews.sort_by(|a, b| a.evaluator.to_string().cmp(&b.evaluator.to_string())); | ||
| for review in reviews { |
There was a problem hiding this comment.
Since ModelId implements Ord, you can compare the evaluators directly instead of converting them to String via to_string(). This avoids unnecessary string allocations during sorting.
reviews.sort_by(|a, b| a.evaluator.cmp(&b.evaluator));| let score = parsed | ||
| .get("score") | ||
| .and_then(|v| v.as_u64().map(|u| u as f64).or_else(|| v.as_f64())) |
There was a problem hiding this comment.
LLMs sometimes return numeric scores as strings in JSON (e.g., "score": "8" instead of "score": 8). To make score parsing more robust and prevent evaluation failures, we should also attempt to parse string values as f64.
| let score = parsed | |
| .get("score") | |
| .and_then(|v| v.as_u64().map(|u| u as f64).or_else(|| v.as_f64())) | |
| let score = parsed | |
| .get("score") | |
| .and_then(|v| { | |
| v.as_u64().map(|u| u as f64) | |
| .or_else(|| v.as_f64()) | |
| .or_else(|| v.as_str().and_then(|s| s.parse::<f64>().ok())) | |
| }) |
| let home = std::env::var("HOME").ok(); | ||
| let mut env_vars: Vec<(&str, &str)> = | ||
| vec![("PI_SKIP_VERSION_CHECK", "1"), ("PI_TELEMETRY", "0")]; | ||
| if let Some(ref h) = home { | ||
| env_vars.push(("HOME", h.as_str())); | ||
| } |
There was a problem hiding this comment.
On Windows, the pi CLI might look for its configuration and credentials in the user's profile directory via the USERPROFILE environment variable rather than HOME. To ensure cross-platform compatibility, we should also preserve and pass USERPROFILE if it is set.
let home = std::env::var("HOME").ok();
let user_profile = std::env::var("USERPROFILE").ok();
let mut env_vars: Vec<(&str, &str)> =
vec![("PI_SKIP_VERSION_CHECK", "1"), ("PI_TELEMETRY", "0")];
if let Some(ref h) = home {
env_vars.push(("HOME", h.as_str()));
}
if let Some(ref up) = user_profile {
env_vars.push(("USERPROFILE", up.as_str()));
}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds configurable brainstorm iteration strategies and wires them through core logic, CLI args, dry-run/JSON/text outputs, and benchmark metadata; implements a Pi CLI provider adapter with tool mapping and JSONL response parsing; updates model-spec parsing, shared schemas, tests, and documentation. ChangesBrainstorm Iteration Strategy and Pi Provider Integration
Sequence DiagramsequenceDiagram
participant User
participant BrainstormCli
participant BrainstormLoop
participant PiProvider
participant Storage
User->>BrainstormCli: brainstorm --iteration-strategy blind
BrainstormCli->>BrainstormLoop: start with BrainstormConfig(iteration_strategy)
BrainstormLoop->>PiProvider: send_message(proposer prompt)
PiProvider-->>BrainstormLoop: assistant text (proposals)
BrainstormLoop->>Storage: save proposals & round artifacts
BrainstormLoop->>PiProvider: send_message(evaluate proposals)
PiProvider-->>BrainstormLoop: evaluation (score + rationale)
BrainstormLoop->>Storage: save evaluation artifacts (include rationale)
BrainstormLoop->>Storage: save metadata.json (iteration_strategy)
BrainstormLoop-->>BrainstormCli: return BrainstormResult (with iteration_strategy)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/refinery_cli/src/commands/common.rs (1)
295-311:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject malformed nested model paths for
pi/opencodein parser.
parse_model_speccurrently accepts invalid specs likepi//gpt-5.4andopencode//glm-5because it only checks the first split. That allows bad input past CLI validation and fails later at provider/runtime.Proposed fix
pub fn parse_model_spec(input: &str) -> Result<ModelId, String> { if input.contains('/') { let (provider, model) = input.split_once('/').unwrap(); if provider.is_empty() || model.is_empty() { return Err(format!("Invalid model spec: '{input}'")); } + if matches!(provider, "pi" | "opencode") { + let Some((subprovider, submodel)) = model.split_once('/') else { + return Err(format!( + "Provider '{provider}' requires '<sub-provider>/<model>', got '{input}'" + )); + }; + if subprovider.is_empty() || submodel.is_empty() { + return Err(format!( + "Provider '{provider}' requires '<sub-provider>/<model>', got '{input}'" + )); + } + } Ok(ModelId::from_parts(provider, model)) } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/refinery_cli/src/commands/common.rs` around lines 295 - 311, The parser accepts malformed nested paths like "pi//gpt-5.4" because parse_model_spec only checks the first split; update the branch that handles input.contains('/') so after let (provider, model) = input.split_once('/').unwrap() you also reject cases where provider or model are empty or either contains additional '/' (e.g., model.contains('/') or provider.contains('/')) or the input contains "//"; return Err(...) for those cases instead of proceeding to ModelId::from_parts, ensuring valid single-slashed specs only.docs/plans/2026-05-23-001-research-brainstorm-strategy-benchmarks-plan.md (1)
11-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the plan header review date for this edit pass.
This plan got a substantive update, but
**Reviewed:**is still2026-05-23. Please refresh it to the current review date using the required header format.As per coding guidelines, "Update plan headers with review dates using format 'Reviewed: YYYY-MM-DD (via
/$SKILL / $COMMAND)'".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-05-23-001-research-brainstorm-strategy-benchmarks-plan.md` around lines 11 - 13, Update the plan header line that currently reads "**Reviewed:** 2026-05-23 (via `/coderabbit / review`)" to the current review date in the required format; replace the date token with today's YYYY-MM-DD and keep the "(via `/$SKILL / $COMMAND`)" suffix (e.g., "**Reviewed:** 2026-05-28 (via `/coderabbit / review`)") so the header for the plan (the line beginning with "**Reviewed:**") reflects the new review timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/refinery_core/src/brainstorm.rs`:
- Around line 349-350: The current manual compare using proposals.sort_by(|(a,
_), (b, _)| a.to_string().cmp(&b.to_string())) should be replaced with
proposals.sort_by_key(|(id, _)| id.to_string()) to satisfy the Clippy
unnecessary_sort_by lint; update the sorting call on the proposals Vec created
from round.proposals.iter().collect() accordingly so it uses sort_by_key with a
closure that returns id.to_string().
- Around line 360-362: Replace the unnecessary sort_by call on reviews with
sort_by_key to satisfy Clippy: instead of reviews.sort_by(|a, b|
a.evaluator.to_string().cmp(&b.evaluator.to_string())), call
reviews.sort_by_key(|r| r.evaluator.to_string()) so the sorting uses the
evaluator string as the key; update the code around the reviews variable in the
brainstorm logic where round.evaluations.get(model_id) is cloned and sorted.
- Around line 313-314: Replace the explicit comparator used in the reviews sort
with a key-based sort to satisfy Clippy: in the block where you clone
entry.reviews into let mut reviews and call reviews.sort_by(|a, b|
a.evaluator.to_string().cmp(&b.evaluator.to_string())), change it to use
reviews.sort_by_key(...) and provide the key extractor that returns
a.evaluator.to_string() (i.e., sort_by_key(|r| r.evaluator.to_string())). This
keeps behavior identical while removing the unnecessary_sort_by lint.
---
Outside diff comments:
In `@crates/refinery_cli/src/commands/common.rs`:
- Around line 295-311: The parser accepts malformed nested paths like
"pi//gpt-5.4" because parse_model_spec only checks the first split; update the
branch that handles input.contains('/') so after let (provider, model) =
input.split_once('/').unwrap() you also reject cases where provider or model are
empty or either contains additional '/' (e.g., model.contains('/') or
provider.contains('/')) or the input contains "//"; return Err(...) for those
cases instead of proceeding to ModelId::from_parts, ensuring valid
single-slashed specs only.
In `@docs/plans/2026-05-23-001-research-brainstorm-strategy-benchmarks-plan.md`:
- Around line 11-13: Update the plan header line that currently reads
"**Reviewed:** 2026-05-23 (via `/coderabbit / review`)" to the current review
date in the required format; replace the date token with today's YYYY-MM-DD and
keep the "(via `/$SKILL / $COMMAND`)" suffix (e.g., "**Reviewed:** 2026-05-28
(via `/coderabbit / review`)") so the header for the plan (the line beginning
with "**Reviewed:**") reflects the new review timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b75bc887-7d30-41ad-9c16-8f97f101369c
📒 Files selected for processing (17)
.env.exampleREADME.mdcrates/refinery_cli/src/commands/README.mdcrates/refinery_cli/src/commands/benchmark_brainstorm.rscrates/refinery_cli/src/commands/brainstorm.rscrates/refinery_cli/src/commands/common.rscrates/refinery_cli/src/commands/converge.rscrates/refinery_cli/src/commands/synthesize.rscrates/refinery_core/src/brainstorm.rscrates/tundish_providers/Cargo.tomlcrates/tundish_providers/src/lib.rscrates/tundish_providers/src/pi.rscrates/tundish_providers/src/tools.rsdocs/HANDOFF.mddocs/plans/2026-05-23-001-research-brainstorm-strategy-benchmarks-plan.mdtodos/013-brainstorm-strategy-benchmarks.mdtodos/022-opencode-concurrency-sqlite-wal.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
.env*
📄 CodeRabbit inference engine (CLAUDE.md)
Copy
.env.exampleto.envand fill in credentials for environment setup
Files:
.env.example
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Runcargo test --workspaceto run all tests
Usecargo build --workspaceto build all crates
Files:
crates/tundish_providers/src/tools.rscrates/refinery_cli/src/commands/converge.rscrates/tundish_providers/src/lib.rscrates/refinery_cli/src/commands/benchmark_brainstorm.rscrates/tundish_providers/src/pi.rscrates/refinery_cli/src/commands/common.rscrates/refinery_cli/src/commands/synthesize.rscrates/refinery_cli/src/commands/brainstorm.rscrates/refinery_core/src/brainstorm.rs
docs/plans/**
📄 CodeRabbit inference engine (CLAUDE.md)
docs/plans/**: Update plan headers with enhancement dates using format 'Enhanced: YYYY-MM-DD (via/deepen-plan)'
Update plan headers with review dates using format 'Reviewed: YYYY-MM-DD (via/$SKILL / $COMMAND)'
Update plan headers with completion dates using format 'Completed: YYYY-MM-DD'
Add addendums to plans using format 'Addendum: YYYY-MM-DD — description of what was added and why'
Create a dedicated plan when picking up a milestone from a roadmap or general plan if the milestone does not have one
Files:
docs/plans/2026-05-23-001-research-brainstorm-strategy-benchmarks-plan.md
docs/HANDOFF.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update
docs/HANDOFF.mdbefore compaction or at session end with project state, architecture gotchas, open work, and review process
Files:
docs/HANDOFF.md
🪛 GitHub Actions: CI / 0_Check.txt
crates/refinery_core/src/brainstorm.rs
[error] 314-314: Clippy (unnecessary_sort_by) failed: use sort_by_key instead of sort_by with a.evaluator.to_string().cmp(&b.evaluator.to_string()). Command: cargo clippy --workspace -- -D warnings.
[error] 350-350: Clippy (unnecessary_sort_by) failed: use sort_by_key instead of sort_by with a.to_string().cmp(&b.to_string()). Command: cargo clippy --workspace -- -D warnings.
[error] 361-361: Clippy (unnecessary_sort_by) failed: use sort_by_key instead of sort_by with a.evaluator.to_string().cmp(&b.evaluator.to_string()). Command: cargo clippy --workspace -- -D warnings.
🪛 GitHub Actions: CI / Check
crates/refinery_core/src/brainstorm.rs
[error] 314-314: Clippy (unnecessary_sort_by / clippy::unnecessary-sort-by): consider using sort_by_key. Replacing reviews.sort_by(|a, b| a.evaluator.to_string().cmp(&b.evaluator.to_string())) with reviews.sort_by_key(|a| a.evaluator.to_string()).
[error] 350-350: Clippy (unnecessary_sort_by / clippy::unnecessary-sort-by): consider using sort_by_key. Replacing proposals.sort_by(|(a, _), (b, _)| a.to_string().cmp(&b.to_string())) with proposals.sort_by_key(|(a, _)| a.to_string()).
[error] 361-361: Clippy (unnecessary_sort_by / clippy::unnecessary-sort-by): consider using sort_by_key. Replacing reviews.sort_by(|a, b| a.evaluator.to_string().cmp(&b.evaluator.to_string())) with reviews.sort_by_key(|a| a.evaluator.to_string()).
🔇 Additional comments (21)
crates/refinery_cli/src/commands/common.rs (1)
27-29: LGTM!Also applies to: 135-137, 529-552
crates/refinery_core/src/brainstorm.rs (7)
17-57: LGTM!
200-243: LGTM!
245-265: LGTM!
389-411: LGTM!
450-455: LGTM!Also applies to: 466-468, 486-498, 599-613, 685-704, 774-788, 804-804, 886-886
896-954: LGTM!
1029-1148: LGTM!crates/tundish_providers/Cargo.toml (1)
9-14: LGTM!crates/tundish_providers/src/lib.rs (2)
39-45: Run full Rust workspace verification before merge.This PR changes multiple Rust files; please run
cargo test --workspaceandcargo build --workspace(not only package-scoped checks) to satisfy repo verification requirements.As per coding guidelines, "
**/*.rs: Runcargo test --workspaceto run all testsandUsecargo build --workspaceto build all crates`."
13-14: LGTM!Also applies to: 30-31, 91-101
crates/tundish_providers/src/pi.rs (4)
38-88: LGTM!
90-135: LGTM!
137-247: LGTM!
248-319: LGTM!crates/tundish_providers/src/tools.rs (1)
43-57: LGTM!Also applies to: 101-107
crates/refinery_cli/src/commands/brainstorm.rs (2)
8-9: LGTM!Also applies to: 35-37, 48-48, 168-168, 179-179, 206-206, 252-252, 312-312
35-37: Workspace Rust checks couldn’t run here—Rust toolchain default is missing.
cargo build --workspace/cargo test --workspacefailed with:rustup could not choose a version of cargo ... no default is configured. Rerun after setting the toolchain (e.g.,rustup default stable) or explicitly (e.g.,cargo +stable build --workspaceandcargo +stable test --workspace) before merge.crates/refinery_cli/src/commands/converge.rs (1)
100-100: LGTM!crates/refinery_cli/src/commands/synthesize.rs (1)
113-113: LGTM!crates/refinery_cli/src/commands/benchmark_brainstorm.rs (1)
47-48: LGTM!Also applies to: 86-89, 135-137, 157-157, 164-174, 476-478
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59e0445226
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut env_vars: Vec<(&str, &str)> = | ||
| vec![("PI_SKIP_VERSION_CHECK", "1"), ("PI_TELEMETRY", "0")]; | ||
| if let Some(ref h) = home { | ||
| env_vars.push(("HOME", h.as_str())); |
There was a problem hiding this comment.
When Pi credentials are supplied through environment variables (Pi's provider docs list OPENAI_API_KEY, ANTHROPIC_API_KEY, custom interpolation variables, etc. as credential sources), spawn_cli clears the child environment and this adapter only re-injects HOME plus telemetry flags. In shells or CI where pi --model openai/... works because the key is in the environment, refinery --models pi/openai/... will run Pi without that key and fail authentication; forward Pi's credential/config env vars or resolve them before spawning.
Useful? React with 👍 / 👎.
|
Addressed the CodeRabbit/GHA feedback in
Local verification:
GitHub Actions and CodeRabbit are passing; Buildkite is still pending/scheduled. |
|
Also addressed the Gemini/Codex feedback in
Re-ran:
|
|
Fixed the GitHub Actions Clippy failure in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5517441c71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env_vars | ||
| } | ||
|
|
||
| const PI_PASSTHROUGH_ENV: &[&str] = &[ |
There was a problem hiding this comment.
In the current allowlist, Pi runs under spawn_cli's cleared environment without several credential sources that Pi documents, such as HF_TOKEN, MINIMAX_CN_API_KEY, and Bedrock role/proxy variables like AWS_CONTAINER_CREDENTIALS_*, AWS_WEB_IDENTITY_TOKEN_FILE, and AWS_ENDPOINT_URL_BEDROCK_RUNTIME (see Pi's Providers docs). In those environments pi --model ... authenticates normally, but the same model through Refinery loses the required variables and fails before producing an answer; extend this passthrough or avoid an allowlist for Pi credential resolution.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification