Fix brainstorm invalid eval triage#46
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 hardens the brainstorm evaluation score parser to support various score formats (such as scaled text, nested objects, and fallback dimension scores) and introduces a bounded response_preview to provider failure records to capture raw response text for easier triage. Documentation, plans, and tests are updated to reflect these changes. A review comment correctly identified a bug in parse_score_text where spaces around a slash (e.g., "8 / 10") would cause the parser to incorrectly return the denominator as the score, and suggested a robust fix using starts_with('/').
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| if let Some((start, end)) = number_spans.first().copied() { | ||
| let after_first = lower[end..].trim_start(); | ||
| if after_first.starts_with("out of") || after_first.starts_with("/10") { |
There was a problem hiding this comment.
If the score text contains spaces around the slash (e.g., "8 / 10" or "8.5 / 10"), after_first.starts_with("/10") will evaluate to false. This causes the parser to fall through and incorrectly return the denominator (10.0) as the score instead of the actual score (8.0 or 8.5).
Changing the check to after_first.starts_with('/') correctly handles any whitespace around the slash and supports other potential denominators (e.g., "/5").
if after_first.starts_with("out of") || after_first.starts_with('/') {![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: 7243d0d800
ℹ️ 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".
| if let Some(score_idx) = lower.rfind("score") { | ||
| if let Some((start, end)) = number_spans | ||
| .iter() | ||
| .copied() | ||
| .find(|(start, _)| *start >= score_idx) | ||
| { | ||
| return trimmed[start..end].parse::<f64>().ok(); |
There was a problem hiding this comment.
When a provider returns a string score that includes the scale after the label, e.g. "score":"Score (1-10): 8" or "Score 1-10: 8", this branch selects the first number after score and records 1.0 as a valid evaluation instead of the actual trailing score. Because the value passes the later 1-10 range check, the brainstorm panel and round histories are silently scored incorrectly rather than treating the response as invalid or using the final score.
Useful? React with 👍 / 👎.
|
Warning Review limit reached
More reviews will be available in 34 minutes and 38 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an optional ChangesBrainstorm evaluation score parser hardening and response_preview capture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
crates/refinery_core/src/brainstorm.rs (1)
1431-1437: 💤 Low valueTest assertion uses byte length instead of char count.
The
invalid_response_previewhelper truncates by character count, but this test asserts onpreview.len()(byte length). For this ASCII-only test case they're equivalent, but the assertion doesn't match the function's semantic contract.Consider using
.chars().count()for clarity:♻️ Suggested change
- assert_eq!(preview.len(), INVALID_RESPONSE_PREVIEW_CHARS); + assert_eq!(preview.chars().count(), INVALID_RESPONSE_PREVIEW_CHARS);🤖 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_core/src/brainstorm.rs` around lines 1431 - 1437, The test `invalid_response_preview_is_bounded` is asserting on byte length using `.len()`, but the `invalid_response_preview` function truncates by character count, not bytes. Change the assertion from `assert_eq!(preview.len(), INVALID_RESPONSE_PREVIEW_CHARS);` to use `preview.chars().count()` instead of `preview.len()` to properly verify that the preview is bounded by the character count specified by `INVALID_RESPONSE_PREVIEW_CHARS`, which matches the actual semantic contract of the function.
🤖 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.
Nitpick comments:
In `@crates/refinery_core/src/brainstorm.rs`:
- Around line 1431-1437: The test `invalid_response_preview_is_bounded` is
asserting on byte length using `.len()`, but the `invalid_response_preview`
function truncates by character count, not bytes. Change the assertion from
`assert_eq!(preview.len(), INVALID_RESPONSE_PREVIEW_CHARS);` to use
`preview.chars().count()` instead of `preview.len()` to properly verify that the
preview is bounded by the character count specified by
`INVALID_RESPONSE_PREVIEW_CHARS`, which matches the actual semantic contract of
the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fab4b42-408f-4753-822a-7f0c82538981
📒 Files selected for processing (6)
crates/refinery_cli/src/commands/brainstorm.rscrates/refinery_core/src/brainstorm.rsdocs/HANDOFF.mddocs/brainstorms/2026-06-12-brainstorm-l3-parser-validation.mddocs/plans/2026-05-23-001-research-brainstorm-strategy-benchmarks-plan.mdtodos/013-brainstorm-strategy-benchmarks.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Runcargo test --workspaceto run all tests
Usecargo build --workspaceto build all crates
Files:
crates/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
🔇 Additional comments (19)
crates/refinery_core/src/brainstorm.rs (13)
137-146: LGTM!
282-289: LGTM!
319-325: LGTM!
327-336: LGTM!
338-367: LGTM!
369-410: LGTM!
412-423: LGTM!
662-702: LGTM!
864-912: LGTM!
1062-1113: LGTM!
1352-1378: LGTM!
1439-1566: LGTM!
1881-1942: LGTM!crates/refinery_cli/src/commands/brainstorm.rs (2)
77-87: LGTM!
383-392: LGTM!docs/HANDOFF.md (1)
5-5: LGTM!Also applies to: 48-48, 62-65, 97-151, 158-159
docs/brainstorms/2026-06-12-brainstorm-l3-parser-validation.md (1)
1-99: LGTM!docs/plans/2026-05-23-001-research-brainstorm-strategy-benchmarks-plan.md (1)
19-20: LGTM!Also applies to: 183-183
todos/013-brainstorm-strategy-benchmarks.md (1)
7-7: LGTM!Also applies to: 107-111
|
Addressed CodeRabbit nitpick in 5d51dce by changing the bounded preview test to assert |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d51dce4f3
ℹ️ 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".
| if let Some((start, end)) = number_spans | ||
| .iter() | ||
| .copied() | ||
| .find(|(start, _)| *start >= score_idx) | ||
| { | ||
| return trimmed[start..end].parse::<f64>().ok(); |
There was a problem hiding this comment.
When the score string has a score label and a scale but no colon before the actual value, e.g. "Score (1-10) 8" or an incomplete "Score (1-10)", this fallback returns the first numeric span after score, so the scale minimum 1 passes the later 1-10 range check and is recorded as a real evaluation score. Fresh evidence since the earlier comment is that the current colon-handling fix still falls into this no-colon branch, silently corrupting brainstorm scores instead of using the trailing value or rejecting the response.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Notes