feat: add brainstorm verb#28
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (97)
📝 WalkthroughSummary by CodeRabbitRelease Notes
I can't reliably rebuild the full hidden review stack with all rangeIds in this message due to length/validation constraints. Please confirm if you'd like me to (a) produce a corrected hidden artifact in multiple parts, or (b) narrow the set of ranges to include (e.g., focus on CLI wiring + core engine + providers). Which option do you prefer? ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: b8f7c8cd70
ℹ️ 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 schema_path = schema.map(|s| { | ||
| let path = std::env::temp_dir() | ||
| .join(format!("refinery-codex-schema-{}.json", std::process::id())); | ||
| (path, s) |
There was a problem hiding this comment.
When a Codex model evaluates multiple peer answers concurrently (the default --max-concurrent 0 lets the evaluate phase spawn them in parallel), every send_message call for that provider writes and later removes the same PID-based schema path. One call can delete or overwrite the file while another codex exec --output-schema invocation is still starting or reading it, causing intermittent schema-file failures during normal multi-model runs; the same PID-only temp-file pattern should be avoided with a unique path per request.
Useful? React with 👍 / 👎.
![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 the brainstorm verb, which implements a multi-round, score-only iteration strategy with controversial selection (quality + evaluator disagreement) to generate diverse panels of answers. The implementation includes new prompt builders, controversy scoring logic, and CLI command orchestration. I have reviewed the code and identified several areas for improvement: JSON serialization error handling is currently silent, the file budget error message is misleading, the retryable flag in error conversion needs to be dynamic, and the answer extraction logic is vulnerable to markdown-wrapped JSON. I have suggested actionable fixes for these issues.
| if let Ok(json) = serde_json::to_string_pretty(&json_output) { | ||
| println!("{json}"); | ||
| } |
There was a problem hiding this comment.
JSON serialization failures are silently swallowed here. If serialization fails, the user receives no output and no error indication. It's better to handle the error explicitly and report it to stderr.
| if let Ok(json) = serde_json::to_string_pretty(&json_output) { | |
| println!("{json}"); | |
| } | |
| match serde_json::to_string_pretty(&json_output) { | |
| Ok(json) => println!("{json}"), | |
| Err(e) => { | |
| eprintln!("Failed to serialize output: {e}"); | |
| return ExitCode::from(1); | |
| } | |
| } |
| if let Ok(json) = serde_json::to_string_pretty(&err) { | ||
| eprintln!("{json}"); | ||
| } |
There was a problem hiding this comment.
Similar to the success path, error serialization failures are silently ignored. While less likely to fail, explicit error handling ensures the user is informed when something goes wrong during the final output phase.
match serde_json::to_string_pretty(&err) {
Ok(json) => eprintln!("{json}"),
Err(ser_err) => eprintln!("Error: {message} (serialization failed: {ser_err})"),
}|
|
||
| let file_size = usize::try_from(meta.len()).unwrap_or(usize::MAX); | ||
| if file_size > budget { | ||
| errors.push(format!("file '{path_str}': exceeds 1MB limit")); |
There was a problem hiding this comment.
The error message is misleading when a prompt is provided alongside files. The budget is reduced by the prompt size, so a file might be rejected even if it's under 1MB if the total exceeds the limit. The message should reflect the actual remaining budget.
| errors.push(format!("file '{path_str}': exceeds 1MB limit")); | |
| errors.push(format!("file '{path_str}': size ({file_size} bytes) exceeds remaining budget ({budget} bytes)")); |
| pub fn converge_error_to_detail(err: &refinery_core::ConvergeError) -> ErrorDetail { | ||
| match err { | ||
| refinery_core::ConvergeError::PhaseFailure { | ||
| phase, | ||
| model, | ||
| source: _, | ||
| } => ErrorDetail { | ||
| code: "phase_failure".to_string(), | ||
| message: err.to_string(), | ||
| provider: Some(model.to_string()), | ||
| round: None, | ||
| phase: Some(phase.to_string()), | ||
| retryable: true, |
There was a problem hiding this comment.
The retryable flag is hardcoded to true for PhaseFailure. However, some provider errors (like BinaryNotFound or MissingCredential) are permanent and should not be retried. You should use the is_permanent() method on the underlying ProviderError to determine this.
| pub fn converge_error_to_detail(err: &refinery_core::ConvergeError) -> ErrorDetail { | |
| match err { | |
| refinery_core::ConvergeError::PhaseFailure { | |
| phase, | |
| model, | |
| source: _, | |
| } => ErrorDetail { | |
| code: "phase_failure".to_string(), | |
| message: err.to_string(), | |
| provider: Some(model.to_string()), | |
| round: None, | |
| phase: Some(phase.to_string()), | |
| retryable: true, | |
| pub fn converge_error_to_detail(err: &refinery_core::ConvergeError) -> ErrorDetail { | |
| match err { | |
| refinery_core::ConvergeError::PhaseFailure { | |
| phase, | |
| model, | |
| source, | |
| } => ErrorDetail { | |
| code: "phase_failure".to_string(), | |
| message: err.to_string(), | |
| provider: Some(model.to_string()), | |
| round: None, | |
| phase: Some(phase.to_string()), | |
| retryable: !source.is_permanent(), | |
| }, |
| fn extract_answer(response: &str) -> Option<String> { | ||
| let parsed: serde_json::Value = serde_json::from_str(response).ok()?; | ||
| parsed | ||
| .get("answer") | ||
| .and_then(|a| a.as_str()) | ||
| .map(String::from) | ||
| } |
There was a problem hiding this comment.
The extract_answer function uses serde_json::from_str directly on the raw response. If a model returns markdown-wrapped JSON (e.g., inside ```json blocks), this will fail. You should use the tundish_core::extract_json utility to strip markdown fences before parsing.
| fn extract_answer(response: &str) -> Option<String> { | |
| let parsed: serde_json::Value = serde_json::from_str(response).ok()?; | |
| parsed | |
| .get("answer") | |
| .and_then(|a| a.as_str()) | |
| .map(String::from) | |
| } | |
| fn extract_answer(response: &str) -> Option<String> { | |
| let json_str = tundish_core::extract_json(response).unwrap_or(response); | |
| let parsed: serde_json::Value = serde_json::from_str(json_str).ok()?; | |
| parsed | |
| .get("answer") | |
| .and_then(|a| a.as_str()) | |
| .map(String::from) | |
| } |
| let synthesis = serde_json::from_str::<serde_json::Value>(&response) | ||
| .ok() | ||
| .and_then(|v| { | ||
| v.get("synthesis") | ||
| .and_then(|s| s.as_str()) | ||
| .map(String::from) | ||
| }) | ||
| .unwrap_or(response); |
There was a problem hiding this comment.
This inlined extraction logic for synthesis also fails to handle markdown-wrapped JSON. It should use tundish_core::extract_json for robustness against common model output quirks.
let json_str = tundish_core::extract_json(&response).unwrap_or(&response);
let synthesis = serde_json::from_str::<serde_json::Value>(json_str)
.ok()
.and_then(|v| {
v.get("synthesis")
.and_then(|s| s.as_str())
.map(String::from)
})
.unwrap_or_else(|| response.clone());![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/tundish_providers/src/gemini.rs (1)
97-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a per-call temp file for
GEMINI_SYSTEM_MD.Line 99 derives the temp path from
std::process::id()only, so concurrentsend_message()calls in the same process all reuse the same file. During evaluate/brainstorm that lets one request overwrite or delete another request’s system prompt before the Gemini subprocess reads it. Use a unique temp file per invocation and keep it alive untilspawn_cli()completes.Also applies to: 120-131
🤖 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/tundish_providers/src/gemini.rs` around lines 97 - 107, The temp file for GEMINI_SYSTEM_MD is currently derived from std::process::id() causing concurrent send_message() calls to collide; change the logic in send_message()/where system_prompt is written so it creates a per-invocation unique temp file (e.g., include a UUID or use tempfile::NamedTempFile) for tmp_path/tmp_path_str, ensure the file is not removed or dropped until after spawn_cli() has been started/awaited so the subprocess can read it, and apply the same fix to the other block that writes the system prompt (the later temp file usage around lines 120-131); reference the system_prompt write, tmp_path/tmp_path_str, and spawn_cli() to locate the changes.crates/refinery_core/src/engine.rs (1)
295-313:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
total_callsis undercounted when a round exits early withInsufficientModels.
call_countis already known, but this error return path bypassesself.total_calls += call_count. Final outcomes can report fewer calls than were actually made.Proposed fix
// Check if enough models produced proposals if proposal_set.proposals.len() < 2 { + self.total_calls += call_count; return Err(ConvergeError::InsufficientModels { round, remaining: proposal_set.proposals.len(), minimum: 2, }); }Also applies to: 393-393
🤖 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/engine.rs` around lines 295 - 313, The early return on InsufficientModels skips adding the round's call count to the engine's accumulator: compute/retain the existing call_count (from proposal_set.proposals.len() + proposal_set.dropped.len()) and ensure self.total_calls is incremented by call_count before returning Err(ConvergeError::InsufficientModels) in the branch that checks proposal_set.proposals.len() < 2; do the same fix for the similar early-return at the other location (around the 393 reference) so every early exit that returns ConvergeError::InsufficientModels updates self.total_calls with call_count first.
🟡 Minor comments (17)
todos/003-verb-synthesize.md-20-20 (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the actual default for
--synthesis-threshold.“default: threshold” is ambiguous and reads like a placeholder. Use the concrete default value (or explicitly reference the exact source default flag/value) to avoid operator confusion.
🤖 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 `@todos/003-verb-synthesize.md` at line 20, The doc entry for the flag `--synthesis-threshold` currently uses the placeholder text "default: threshold" which is ambiguous; update that line to show the concrete default value (e.g., "default: 0.5") or explicitly reference the canonical source constant or flag name (for example, DEFAULT_SYNTHESIS_THRESHOLD or SynthesisConfig.SYNTHESIS_THRESHOLD_DEFAULT) so operators see the exact default; change the text for `--synthesis-threshold` to either the numeric default or a clear pointer to the source where the default is defined.docs/plans/2026-03-17-refactor-cli-subcommand-converge-plan.md-27-31 (1)
27-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced code block on Line 27.
This currently triggers markdownlint MD040.
💡 Proposed fix
-``` +```text refinery <COMMAND> converge Reach consensus across multiple models help Print help</details> <details> <summary>🤖 Prompt for AI Agents</summary>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-03-17-refactor-cli-subcommand-converge-plan.mdaround lines
27 - 31, Add a language tag to the fenced code block containing the CLI usage
snippet (the block starting with "refinery ") to satisfy markdownlint
MD040; update the opening fence to include an appropriate language identifier
such as text (e.g., change "" to "text") so the block that shows "refinery
/ converge / help" is properly tagged.</details> </blockquote></details> <details> <summary>docs/plans/2026-03-17-refactor-cli-subcommand-converge-plan.md-1-6 (1)</summary><blockquote> `1-6`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix invalid YAML frontmatter key on Line 5.** `**Completed:** 2026-03-17` is markdown formatting inside frontmatter and can break metadata parsers. Use a plain YAML key instead. <details> <summary>💡 Proposed fix</summary> ```diff --- title: "refactor: Restructure CLI into refinery converge subcommand" type: refactor date: 2026-03-17 -**Completed:** 2026-03-17 +completed: 2026-03-17 --- ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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-03-17-refactor-cli-subcommand-converge-plan.md` around lines 1 - 6, The YAML frontmatter contains a markdown-formatted key "**Completed:** 2026-03-17" which can break parsers; replace that markdown-styled line with a plain YAML key like completed: "2026-03-17" (or Completed: 2026-03-17) so the frontmatter is valid YAML—update the header block in this file's frontmatter to remove markdown formatting and use a simple key:value pair. ``` </details> </blockquote></details> <details> <summary>docs/brainstorms/2026-03-18-synthesize-verb-requirements.md-51-54 (1)</summary><blockquote> `51-54`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix markdown reference-label syntax in outstanding questions.** `[Technical]` is parsed as a reference link label and currently has no definition, triggering MD052. Use plain text `(Technical)` (or define link refs) to avoid lint noise. <details> <summary>Proposed doc-only fix</summary> ```diff -- [Affects R3][Technical] How to structure the synthesis prompt — should qualifying answers be presented anonymously (like converge evaluations) or attributed? -- [Affects R4][Technical] What JSON schema to use for synthesis evaluation — extend EVALUATE_SCHEMA or create SYNTHESIS_EVAL_SCHEMA? -- [Affects R5][Technical] Should the synthesis phase use the same convergence/tiebreaking logic as converge, or simpler "highest score wins"? -- [Affects R1][Technical] Can we reuse `Engine::run()` for the converge phase and then add synthesis as a post-processing step, or does the engine need a new method? +- [Affects R3] (Technical) How to structure the synthesis prompt — should qualifying answers be presented anonymously (like converge evaluations) or attributed? +- [Affects R4] (Technical) What JSON schema to use for synthesis evaluation — extend EVALUATE_SCHEMA or create SYNTHESIS_EVAL_SCHEMA? +- [Affects R5] (Technical) Should the synthesis phase use the same convergence/tiebreaking logic as converge, or simpler "highest score wins"? +- [Affects R1] (Technical) Can we reuse `Engine::run()` for the converge phase and then add synthesis as a post-processing step, or does the engine need a new method? ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/brainstorms/2026-03-18-synthesize-verb-requirements.md` around lines 51 - 54, Replace the ambiguous markdown reference labels like "[Technical]" in the bullet list with plain parenthetical text "(Technical)" to avoid MD052 link-label parsing; update each question line that mentions items such as "How to structure the synthesis prompt", "What JSON schema to use for synthesis evaluation — extend EVALUATE_SCHEMA or create SYNTHESIS_EVAL_SCHEMA?", "Should the synthesis phase use the same convergence/tiebreaking logic...", and "Can we reuse Engine::run()..." so the tag appears as "(Technical)" rather than "[Technical]". Ensure all occurrences in that section are changed (or alternatively add explicit link reference definitions) so the linter no longer treats those tags as undefined reference links. ``` </details> </blockquote></details> <details> <summary>docs/plans/2026-03-18-001-feat-synthesize-verb-plan.md-94-115 (1)</summary><blockquote> `94-115`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language tag to the fenced block for markdownlint compliance.** Line 94 opens a code fence without a language, triggering MD040. <details> <summary>Proposed fix</summary> ```diff -``` +```text 1. Parse args, build providers (same as converge) 2. Run converge phase: ... 8. Return result with synthesis as the answer ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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-03-18-001-feat-synthesize-verb-plan.mdaround lines 94 -
115, The markdown code fence in the plan's numbered list is missing a language
tag which triggers MD040; update the opening fence fromto include a language (for example,text or ```md) so markdownlint passes—modify the
fenced block that contains the numbered steps (the "Parse args, build
providers..." through "Return result with synthesis as the answer") to use a
tagged fence.</details> </blockquote></details> <details> <summary>docs/brainstorms/2026-03-30-brainstorm-verb-requirements.md-48-54 (1)</summary><blockquote> `48-54`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix undefined markdown reference-style labels in bracketed tags.** Line 48–Line 54 use `[Technical]` / `[Needs research]`, which markdownlint treats as undefined reference labels (MD052). <details> <summary>Proposed fix</summary> ```diff -- [Affects R4][Technical] Concrete formula for controversy scoring — Reddit's `upvotes/(upvotes+downvotes)` needs adaptation since we have continuous scores (1-10), not binary up/down. Score variance (standard deviation) is the simplest proxy. Are there better formulas? -- [Affects R5][Technical] How to select the final panel — top N by controversy score? Or cluster by controversy score and pick representatives? Simple top-N is the v0 default. -- [Affects R8][Technical] What dimensions should the brainstorm evaluation rubric score on? Candidates: originality, insight, depth, provocativeness, feasibility. Needs to differ from converge's accuracy/correctness focus. -- [Affects R2][Technical] Prompt structure for score-only iteration — how to present a model's own prior answers + scores without directing improvement. "Here are your previous attempts and how they were received" vs more neutral framing. -- [Affects R7][Needs research] What's the right default for `--max-rounds`? Brainstorming needs more rounds than converge (exploration vs convergence), but too many rounds may cause models to exhaust their variation. Likely 5-10. -- [Affects R5][Needs research] What's the right default for `--panel-size`? Too small (2) isn't a panel. Too large (10) dilutes quality. Likely 3-5. -- [Affects R1][Technical] Can we reuse `Engine::run()` for the round loop, or does score-only iteration require a different engine mode? The key difference: models don't see round context (other answers + evaluations), only their own history. +- [Affects R4] *(Technical)* Concrete formula for controversy scoring — Reddit's `upvotes/(upvotes+downvotes)` needs adaptation since we have continuous scores (1-10), not binary up/down. Score variance (standard deviation) is the simplest proxy. Are there better formulas? +- [Affects R5] *(Technical)* How to select the final panel — top N by controversy score? Or cluster by controversy score and pick representatives? Simple top-N is the v0 default. +- [Affects R8] *(Technical)* What dimensions should the brainstorm evaluation rubric score on? Candidates: originality, insight, depth, provocativeness, feasibility. Needs to differ from converge's accuracy/correctness focus. +- [Affects R2] *(Technical)* Prompt structure for score-only iteration — how to present a model's own prior answers + scores without directing improvement. "Here are your previous attempts and how they were received" vs more neutral framing. +- [Affects R7] *(Needs research)* What's the right default for `--max-rounds`? Brainstorming needs more rounds than converge (exploration vs convergence), but too many rounds may cause models to exhaust their variation. Likely 5-10. +- [Affects R5] *(Needs research)* What's the right default for `--panel-size`? Too small (2) isn't a panel. Too large (10) dilutes quality. Likely 3-5. +- [Affects R1] *(Technical)* Can we reuse `Engine::run()` for the round loop, or does score-only iteration require a different engine mode? The key difference: models don't see round context (other answers + evaluations), only their own history. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/brainstorms/2026-03-30-brainstorm-verb-requirements.md` around lines 48 - 54, The bracketed labels like [Technical] and [Needs research] are being parsed as undefined markdown reference-style links (MD052); replace those square-bracket-only tags with inline text or parentheses so they aren't treated as reference links—for example change occurrences such as "[Affects R4][Technical]" and "[Affects R7][Needs research]" to "[Affects R4] (Technical)" or "[Affects R4] - Technical" (and similarly for R5, R8, R2, R1), ensuring all instances of the symbols [Technical] and [Needs research] are converted consistently. ``` </details> </blockquote></details> <details> <summary>crates/tundish_providers/src/lib.rs-76-80 (1)</summary><blockquote> `76-80`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Unknown-provider error lists providers that may not be compiled in.** The message is hardcoded, so with feature-gated builds it can advertise unavailable providers. Build the supported list from `#[cfg(feature = "...")]` constants to keep runtime errors accurate. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/tundish_providers/src/lib.rs` around lines 76 - 80, The error message for the unknown provider in the ProviderError::ProcessFailed block currently hardcodes the supported list; update it to construct the supported providers string from per-provider feature-gated constants (e.g., define constants like SUPPORTED_PROVIDER_CLAUDE_CODE, SUPPORTED_PROVIDER_CODEX_CLI, SUPPORTED_PROVIDER_GEMINI_CLI, SUPPORTED_PROVIDER_OPENCODE behind #[cfg(feature = "...")]) and join those constants at runtime to produce the message using model_id and the incoming other value, so the Unknown provider message only advertises providers actually compiled into the binary. ``` </details> </blockquote></details> <details> <summary>docs/plans/2026-03-14-feat-indicatif-multi-spinner-comfy-table-plan.md-27-67 (1)</summary><blockquote> `27-67`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add language identifiers to fenced code blocks.** Markdownlint MD040 will keep warning on these unlabeled fences. Tag them (e.g., `text`) to keep docs lint-clean. <details> <summary>Suggested patch</summary> ```diff -``` +```text Round 2/5 ... -``` +``` -``` +```text tundish ProgressFn → SpinnerState (Mutex) → tick task (single line eprint!) refinery ProgressFn → render_progress() → SpinnerState → tick task -``` +``` -``` +```text tundish ProgressFn → indicatif ProgressBar per model (set_message) refinery ProgressFn → indicatif MultiProgress (add/finish bars) Score table → comfy-table (printed above MultiProgress) -``` +``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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-03-14-feat-indicatif-multi-spinner-comfy-table-plan.md
around lines 27 - 67, Several fenced code blocks in the plan lack language
identifiers (triggering MD040); add a language tag (e.g., text) to each
triple-backtick fence that surrounds the score table block starting with "Round
2/5", the flow block containing "tundish ProgressFn → SpinnerState (Mutex) →
tick task", and the "New flow" block containing "tundish ProgressFn → indicatif
ProgressBar per model (set_message)" so the markdown linter stops warning;
update each opening fence fromtotext and keep the matching closing
fence unchanged.</details> </blockquote></details> <details> <summary>docs/solutions/integration-issues/opencode-provider-integration.md-35-80 (1)</summary><blockquote> `35-80`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Label fenced code blocks with a language.** Two code fences are unlabeled, which triggers MD040 and can fail docs lint workflows. <details> <summary>Suggested patch</summary> ```diff -``` +```text refinery provider: opencode refinery model: kimi-for-coding/kimi-k2-thinking opencode --model: kimi-for-coding/kimi-k2-thinking -``` +``` -``` +```text opencode/minimax-m2.5-free ... zai-coding-plan/glm-5 -``` +``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/solutions/integration-issues/opencode-provider-integration.mdaround
lines 35 - 80, Two fenced code blocks in the docs are missing language labels
(the block starting with "refinery provider: opencode / refinery model:
kimi-for-coding/..." and the final examples block listing
"opencode/minimax-m2.5-free ... zai-coding-plan/glm-5"); update each opening
triple-backtick to include a language (e.g.,text) so they becometext ...unchanged.docs/plans/2026-03-13-refactor-remove-refine-phase-plan.md-60-60 (1)
60-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
Lines 60 and 114 violate MD040 (
fenced-code-language) from the provided static analysis hints.📝 Proposed fix
-``` +```xml <your_history> <round number="1"> <your_proposal> @@ </round> </your_history> -``` +``` -``` +```text engine.rs ├─→ phases::refine::run() [DELETE call] @@ types.rs ├─→ RefinementSet definition [DELETE] ├─→ RoundOutcome.refinements [DELETE field] ├─→ Phase::Refine variant [DELETE] └─→ Cost formula [UPDATE N²+N → N²] -``` +```Also applies to: 114-114
🤖 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-03-13-refactor-remove-refine-phase-plan.md` at line 60, Add explicit language identifiers to the fenced code blocks that currently lack them: change the opening fence before the XML snippet to "```xml" (the block containing <your_history> / <round number="1"> / <your_proposal> ... </your_history>) and change the opening fence before the plain file-tree snippet to "```text" (the block showing engine.rs, types.rs, etc. with DELETE/UPDATE notes); apply the same fix for the other occurrence at line 114 so both MD040 violations are resolved.crates/refinery_cli/src/progress.rs-42-45 (1)
42-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGate all ANSI control output behind a TTY check, not just spinner ticks.
start_tick()disables animation on non-TTY, but other event methods still emit\r\x1b[2Kand color escapes. That leaks raw control codes into redirected logs/pipes. Please apply the same TTY gate to event rendering paths (or centralize rendering with ANSI/no-ANSI variants). Also update the docs claim about non-TTY graceful degradation once this is fixed.🤖 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/progress.rs` around lines 42 - 45, The spinner TTY check in start_tick(...) only disables the JoinHandle but other methods still emit ANSI sequences like "\r\x1b[2K" and color escapes to stderr; modify all rendering paths that write to stderr (centralize in a renderer used by start_tick, or add explicit TTY guards in each event method) so they early-return or use a no-ANSI variant when std::io::stderr().is_terminal() is false or when self.hidden is true; ensure any functions that emit control codes (the event rendering functions called by start_tick and other progress/event methods) consult the same TTY check and update the public doc string that currently claims graceful non-TTY degradation to reflect the fixed behavior.crates/refinery_cli/src/progress.rs-166-171 (1)
166-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd deterministic tie-breaker for equal round scores in table rendering.
When scores compare equal, the current comparator returns
Equal, so order depends onHashMapkey iteration. Add a secondary compare on model name for stable output.♻️ Suggested deterministic comparator
- models.sort_by(|a, b| { - latest - .get(*b) - .partial_cmp(&latest.get(*a)) - .unwrap_or(std::cmp::Ordering::Equal) - }); + models.sort_by(|a, b| { + latest + .get(*b) + .partial_cmp(&latest.get(*a)) + .unwrap_or(std::cmp::Ordering::Equal) + .then_with(|| a.cmp(b)) + });🤖 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/progress.rs` around lines 166 - 171, The sort comparator for `models.sort_by(|a, b| { ... })` currently returns Equal when `latest.get(*a)` and `latest.get(*b)` are equal, causing nondeterministic iteration order; change the comparator to use a deterministic tie-breaker by chaining a secondary comparison on the model identifier (e.g., `a.cmp(b)` or the model name string) when the primary `partial_cmp` yields Equal — ensure you use the same `latest.get(*)` primary compare (with `unwrap_or(Ordering::Equal)`) and then return the result of the name comparison when equal so table rendering is stable.README.md-200-200 (1)
200-200:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLabel the example output fences with a language.
These unlabeled fenced blocks trigger markdownlint MD040.
Suggested fix pattern
- ``` + ```text $ refinery converge ... ... }</details> Also applies to: 276-276, 285-285, 375-375 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README.mdat line 200, Replace unlabeled fenced code blocks showing example
CLI output with language-labeled fences (usetext) so markdownlint MD040 is satisfied; specifically update the block that begins with the example output line "$ refinery converge ..." to start with "text" instead of "```", and
apply the same change to the other unlabeled example-output fences noted (the
blocks near the other example outputs). Ensure only the fence markers are
changed (no content modifications) so the examples render as plain text.</details> </blockquote></details> <details> <summary>docs/plans/2026-03-13-refactor-provider-model-syntax-plan.md-172-172 (1)</summary><blockquote> `172-172`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language tag to the dependency-chain code fence.** The unlabeled fence triggers markdownlint MD040. <details> <summary>Suggested fix</summary> ```diff -``` +```text types.rs ├─→ ModelId(String) [REPLACE with struct { provider, model }] ... ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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-03-13-refactor-provider-model-syntax-plan.mdat line 172,
The code fence that shows the dependency-chain snippet (the unlabeled
triple-backtick block containing "types.rs" and the "ModelId(String) ..." lines)
should be changed to include a language tag to satisfy markdownlint MD040;
update the opening fence fromtotext (or another appropriate language
tag) so the block becomes a labeled code fence (e.g., ```text) while leaving the
fence contents unchanged.</details> </blockquote></details> <details> <summary>docs/solutions/logic-errors/consensus-tiebreaking-and-winner-semantics.md-17-17 (1)</summary><blockquote> `17-17`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Specify a language for the fenced code block.** This fence is missing a language token and triggers markdownlint (MD040). <details> <summary>Suggested fix</summary> ```diff -``` +```text R1: opus=8.8 ★, kimi=8.8 (opus wins by luck) R2: kimi=9.2 ★, opus=9.0 (kimi wins legitimately) R3: glm=9.0 ★, kimi=9.0 (glm wins by luck) ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/solutions/logic-errors/consensus-tiebreaking-and-winner-semantics.mdat
line 17, The fenced code block containing the example lines starting with "R1:
opus=8.8 ★, kimi=8.8" is missing a language token and triggers markdownlint
MD040; update that fence to include a language (e.g., add "text" so the opening
fence becomes ```text) so the block is explicitly marked and the linter error is
resolved.</details> </blockquote></details> <details> <summary>crates/refinery_cli/src/commands/common.rs-269-273 (1)</summary><blockquote> `269-273`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix the recovery hint for shorthand provider names.** Line 271 formats every shorthand as both `{input}-code` and `{input}-cli`, but only one of those is valid for each provider. For example, `claude` suggests nonexistent `claude-cli`, and `codex` suggests nonexistent `codex-code`. This is the error path users hit when migrating to the new syntax, so the hint should point to the real provider name for each alias. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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 269 - 273, The recovery hint incorrectly suggests both "{input}-code" and "{input}-cli" for every shorthand; update the error branch that matches "claude" | "codex" | "gemini" (the match arm handling provider parsing using the variable input) so each shorthand maps to its real provider name: suggest "claude-code" for "claude", "codex-cli" for "codex", and "gemini-cli" for "gemini" (you can implement this by replacing the static combined hint with a small match/map on input that inserts the correct suggestion into the process-friendly Err string). ``` </details> </blockquote></details> <details> <summary>crates/refinery_cli/src/commands/converge.rs-41-47 (1)</summary><blockquote> `41-47`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Keep early validation/config errors JSON-shaped in JSON mode.** When `--output-format json` is selected, these branches still emit plain text errors. That breaks machine-readable output contracts. Also applies to: 59-75 <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/converge.rs` around lines 41 - 47, The plain-text early-exit error for the stability/max-rounds check needs to be JSON-formatted when the user requested JSON output; update the branch that currently calls eprintln!(...) and returns ExitCode::from(4) to inspect args.output_format (e.g. compare to OutputFormat::Json) and emit a structured JSON error (use serde_json::json or serde_json::to_string to produce something like { "error": "…", "details": { "stability_rounds": ..., "max_rounds": ... } }) instead of plain text, then return the same ExitCode; make the same change for the other validation branches referenced (the branches around the 59-75 area) so all early validation/config errors (those using eprintln! and ExitCode::from) produce machine-readable JSON when args.output_format == OutputFormat::Json. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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_cli/src/commands/brainstorm.rs:
- Around line 178-181: The current uses of
serde_json::to_string_pretty(&json_output) in brainstorm.rs (the success path at
the shown block and the error-reporting path around lines 219-221) ignore Err
results and can silently drop output; update both call sites (the success path
where you println!("{json}") and the error-report path) to match on the Result:
on Ok(json_string) print it, but on Err(e) either return/propagate an error
(fail loudly) in the success path (e.g., return Err(e) or panic with context)
and in the error-report path fall back to a plain-text representation of the
data (e.g., debug/Display formatting of json_output) while also logging the
serialization error. Ensure you reference and modify the
serde_json::to_string_pretty(&json_output) calls and the surrounding
success/error handling so failures are not silently ignored.In
@crates/refinery_cli/src/commands/synthesize.rs:
- Around line 500-503: The selection of
bestfrommean_scoresis
non-deterministic on ties because you only compare floating scores; update the
comparator used in themean_scores.iter().max_by(...)call to include a
deterministic secondary key (for example the synthesis identifier or index) so
ties resolve consistently—compare (score, id) or (score, Reverse(id)) as the
tuple comparator or switch tomax_by_key/sort_byusing (partial_cmp(score),
id) logic; ensure you handlef64::NAN/partial_cmp the same way as before but
include the deterministic key (refer tomean_scores, the.iter().max_by
call, and thebestvariable).In
@crates/refinery_core/src/brainstorm.rs:
- Around line 149-151: The bug is that latest_answers retains entries from prior
rounds when a model fails to propose, letting stale answers influence the final
panel; instead of inserting into the existing latest_answers map, replace or
clear it before populating from the current round. Concretely, in the loop that
processes round_proposals (the block that currently does for (model_id, answer)
in &round_proposals { latest_answers.insert(...) }), either call
latest_answers.clear() then insert the current round entries, or rebuild
latest_answers = round_proposals.iter().map(|(k,v)| (k.clone(),
v.clone())).collect::<...>() so only current-round proposals remain. Apply the
same change to the other spots where latest_answers is updated (the other
similar blocks handling round_proposals/round scores) so stale candidates cannot
persist into final panel selection.- Around line 229-236: The current parsing in brainstorm.rs uses
serde_json::from_str(&response).unwrap_or_default(), which silently fails when
the provider wraps the JSON (e.g., fenced code blocks) and drops valid scores;
replace this with a hardened parse flow: try serde_json::from_str(&response) and
if it errors, attempt to clean the response (strip surrounding fences/backticks
and whitespace, and if needed locate the first '{' and last '}' to extract a
JSON substring) and re-run serde_json::from_str on the cleaned string, handling
Result errors instead of unwrap_or_default; then compute score_val exactly as
before (the get("score").and_then(...).filter(...)) from the successfully parsed
serde_json::Value and proceed only if Some(score), ensuring no panics and
preserving valid wrapped JSON responses.In
@crates/refinery_core/src/prompts/mod.rs:
- Around line 41-44: The wrap_answer function currently interpolates model_label
(and elsewhere reviewer/label strings at the noted 160-165 region) directly into
an XML attribute, which can break tag boundaries if values contain quotes or
angle brackets; fix by escaping attribute values before interpolation (e.g., add
or use a helper like escape_xml_attr that replaces & with &, < with <, >
with >, " with ", and ' with '), call that helper on model_label
(and the reviewer/label variables used in the other block) instead of the raw
strings, and then use the escaped results in the format! macro while keeping
sanitize_for_delimiter(answer, nonce) unchanged.- Around line 49-52: The code in shuffled_labels (the labels: Vec
mapping) currently uses u8::try_from(i).expect(...) which will panic when i >=
256; change the label generation to never panic by computing the letter index
without try_from (e.g., use i as usize and map it into the alphabet range like
(i % 26) so letters wrap after 'Z'), then build the char from b'A' + (index as
u8) or use char::from_u32(('A' as u32) + (index as u32)). Update the closure
creating labels (the map in shuffled_labels that references char::from and
format!("Answer {c}")) to use this safe index calculation so large count values
no longer cause a panic.In
@crates/refinery_core/src/scoring.rs:
- Around line 57-61: The sort currently only compares controversy_score and
mean_score so equal scores leave order non-deterministic; update the closure
used in candidates.sort_by to add a deterministic tertiary tie-breaker by
comparing model_id (e.g., add .then_with(|| a.model_id.cmp(&b.model_id)) or
.then_with(|| b.model_id.cmp(&a.model_id)) to the existing chain). Ensure the
field name model_id on the candidate type implements Ord/PartialOrd (or convert
to a comparable key like &str) so the tertiary comparison compiles and yields
reproducible ordering.In
@crates/tundish_core/src/error.rs:
- Around line 55-62: The permanent-error detection in the Self::ProcessFailed
branch is brittle and case-sensitive; normalize the error text (e.g., to
lowercase) and test against a small set of normalized tokens/phrases (like "not
found", "not supported", "not exist", "model", "authentication", "auth") or
equivalent regexes to catch common variants and hyphenation/whitespace
differences, then use that normalized-check logic in the ProcessFailed arm in
error.rs so the permanent vs transient decision is case-insensitive and less
fragile.In
@crates/tundish_providers/src/codex.rs:
- Around line 125-140: The code currently builds a temp file path for
per-request schemas using only std::process::id() (the schema_path variable and
its usage) which causes collisions across concurrent calls; change this to
create a unique per-invocation temporary file (e.g., use tempfile::NamedTempFile
or create a file with a UUID/timestamp+thread id), write the schema into that
NamedTempFile (keep the NamedTempFile alive so the file is not deleted while the
Codex subprocess reads it), and pass the NamedTempFile.path() string where
schema_path_str is used; apply the same change to the other similar block
referenced (the later schema-writing logic around the second
schema_path/schema_path_str usage) so each send_message() invocation uses its
own unique temp file that is removed only after the subprocess finishes.In
@crates/tundish_providers/src/process.rs:
- Around line 20-29: sanitized_path() currently preserves relative PATH entries
(like "./bin" or "node_modules/.bin") which lets workspace-local executables be
found before env_clear() runs; update sanitized_path() to filter out any
non-absolute paths (keep only entries where Path::is_absolute() is true and
non-empty) and return a PATH composed solely of absolute directories, and then
ensure the code that resolves provider executables (the place that invokes
"which" / resolves the provider binary) uses this same sanitized_path() value
rather than the ambient PATH so binary resolution is performed against the
absolute-only PATH you pass to child processes.
Outside diff comments:
In@crates/refinery_core/src/engine.rs:
- Around line 295-313: The early return on InsufficientModels skips adding the
round's call count to the engine's accumulator: compute/retain the existing
call_count (from proposal_set.proposals.len() + proposal_set.dropped.len()) and
ensure self.total_calls is incremented by call_count before returning
Err(ConvergeError::InsufficientModels) in the branch that checks
proposal_set.proposals.len() < 2; do the same fix for the similar early-return
at the other location (around the 393 reference) so every early exit that
returns ConvergeError::InsufficientModels updates self.total_calls with
call_count first.In
@crates/tundish_providers/src/gemini.rs:
- Around line 97-107: The temp file for GEMINI_SYSTEM_MD is currently derived
from std::process::id() causing concurrent send_message() calls to collide;
change the logic in send_message()/where system_prompt is written so it creates
a per-invocation unique temp file (e.g., include a UUID or use
tempfile::NamedTempFile) for tmp_path/tmp_path_str, ensure the file is not
removed or dropped until after spawn_cli() has been started/awaited so the
subprocess can read it, and apply the same fix to the other block that writes
the system prompt (the later temp file usage around lines 120-131); reference
the system_prompt write, tmp_path/tmp_path_str, and spawn_cli() to locate the
changes.
Minor comments:
In@crates/refinery_cli/src/commands/common.rs:
- Around line 269-273: The recovery hint incorrectly suggests both
"{input}-code" and "{input}-cli" for every shorthand; update the error branch
that matches "claude" | "codex" | "gemini" (the match arm handling provider
parsing using the variable input) so each shorthand maps to its real provider
name: suggest "claude-code" for "claude", "codex-cli" for "codex", and
"gemini-cli" for "gemini" (you can implement this by replacing the static
combined hint with a small match/map on input that inserts the correct
suggestion into the process-friendly Err string).In
@crates/refinery_cli/src/commands/converge.rs:
- Around line 41-47: The plain-text early-exit error for the
stability/max-rounds check needs to be JSON-formatted when the user requested
JSON output; update the branch that currently calls eprintln!(...) and returns
ExitCode::from(4) to inspect args.output_format (e.g. compare to
OutputFormat::Json) and emit a structured JSON error (use serde_json::json or
serde_json::to_string to produce something like { "error": "…", "details": {
"stability_rounds": ..., "max_rounds": ... } }) instead of plain text, then
return the same ExitCode; make the same change for the other validation branches
referenced (the branches around the 59-75 area) so all early validation/config
errors (those using eprintln! and ExitCode::from) produce machine-readable JSON
when args.output_format == OutputFormat::Json.In
@crates/refinery_cli/src/progress.rs:
- Around line 42-45: The spinner TTY check in start_tick(...) only disables the
JoinHandle but other methods still emit ANSI sequences like "\r\x1b[2K" and
color escapes to stderr; modify all rendering paths that write to stderr
(centralize in a renderer used by start_tick, or add explicit TTY guards in each
event method) so they early-return or use a no-ANSI variant when
std::io::stderr().is_terminal() is false or when self.hidden is true; ensure any
functions that emit control codes (the event rendering functions called by
start_tick and other progress/event methods) consult the same TTY check and
update the public doc string that currently claims graceful non-TTY degradation
to reflect the fixed behavior.- Around line 166-171: The sort comparator for
models.sort_by(|a, b| { ... })
currently returns Equal whenlatest.get(*a)andlatest.get(*b)are equal,
causing nondeterministic iteration order; change the comparator to use a
deterministic tie-breaker by chaining a secondary comparison on the model
identifier (e.g.,a.cmp(b)or the model name string) when the primary
partial_cmpyields Equal — ensure you use the samelatest.get(*)primary
compare (withunwrap_or(Ordering::Equal)) and then return the result of the
name comparison when equal so table rendering is stable.In
@crates/tundish_providers/src/lib.rs:
- Around line 76-80: The error message for the unknown provider in the
ProviderError::ProcessFailed block currently hardcodes the supported list;
update it to construct the supported providers string from per-provider
feature-gated constants (e.g., define constants like
SUPPORTED_PROVIDER_CLAUDE_CODE, SUPPORTED_PROVIDER_CODEX_CLI,
SUPPORTED_PROVIDER_GEMINI_CLI, SUPPORTED_PROVIDER_OPENCODE behind #[cfg(feature
= "...")]) and join those constants at runtime to produce the message using
model_id and the incoming other value, so the Unknown provider message only
advertises providers actually compiled into the binary.In
@docs/brainstorms/2026-03-18-synthesize-verb-requirements.md:
- Around line 51-54: Replace the ambiguous markdown reference labels like
"[Technical]" in the bullet list with plain parenthetical text "(Technical)" to
avoid MD052 link-label parsing; update each question line that mentions items
such as "How to structure the synthesis prompt", "What JSON schema to use for
synthesis evaluation — extend EVALUATE_SCHEMA or create SYNTHESIS_EVAL_SCHEMA?",
"Should the synthesis phase use the same convergence/tiebreaking logic...", and
"Can we reuse Engine::run()..." so the tag appears as "(Technical)" rather than
"[Technical]". Ensure all occurrences in that section are changed (or
alternatively add explicit link reference definitions) so the linter no longer
treats those tags as undefined reference links.In
@docs/brainstorms/2026-03-30-brainstorm-verb-requirements.md:
- Around line 48-54: The bracketed labels like [Technical] and [Needs research]
are being parsed as undefined markdown reference-style links (MD052); replace
those square-bracket-only tags with inline text or parentheses so they aren't
treated as reference links—for example change occurrences such as "[Affects
R4][Technical]" and "[Affects R7][Needs research]" to "[Affects R4] (Technical)"
or "[Affects R4] - Technical" (and similarly for R5, R8, R2, R1), ensuring all
instances of the symbols [Technical] and [Needs research] are converted
consistently.In
@docs/plans/2026-03-13-refactor-provider-model-syntax-plan.md:
- Line 172: The code fence that shows the dependency-chain snippet (the
unlabeled triple-backtick block containing "types.rs" and the "ModelId(String)
..." lines) should be changed to include a language tag to satisfy markdownlint
MD040; update the opening fence fromtotext (or another appropriate
language tag) so the block becomes a labeled code fence (e.g., ```text) while
leaving the fence contents unchanged.In
@docs/plans/2026-03-13-refactor-remove-refine-phase-plan.md:
- Line 60: Add explicit language identifiers to the fenced code blocks that
currently lack them: change the opening fence before the XML snippet to "xml" (the block containing <your_history> / <round number="1"> / <your_proposal> ... </your_history>) and change the opening fence before the plain file-tree snippet to "text" (the block showing engine.rs, types.rs, etc. with DELETE/UPDATE
notes); apply the same fix for the other occurrence at line 114 so both MD040
violations are resolved.In
@docs/plans/2026-03-14-feat-indicatif-multi-spinner-comfy-table-plan.md:
- Around line 27-67: Several fenced code blocks in the plan lack language
identifiers (triggering MD040); add a language tag (e.g., text) to each
triple-backtick fence that surrounds the score table block starting with "Round
2/5", the flow block containing "tundish ProgressFn → SpinnerState (Mutex) →
tick task", and the "New flow" block containing "tundish ProgressFn → indicatif
ProgressBar per model (set_message)" so the markdown linter stops warning;
update each opening fence fromtotext and keep the matching closing
fence unchanged.In
@docs/plans/2026-03-17-refactor-cli-subcommand-converge-plan.md:
- Around line 27-31: Add a language tag to the fenced code block containing the
CLI usage snippet (the block starting with "refinery ") to satisfy
markdownlint MD040; update the opening fence to include an appropriate language
identifier such as text (e.g., change "" to "text") so the block that
shows "refinery / converge / help" is properly tagged.- Around line 1-6: The YAML frontmatter contains a markdown-formatted key
"Completed: 2026-03-17" which can break parsers; replace that
markdown-styled line with a plain YAML key like completed: "2026-03-17" (or
Completed: 2026-03-17) so the frontmatter is valid YAML—update the header block
in this file's frontmatter to remove markdown formatting and use a simple
key:value pair.In
@docs/plans/2026-03-18-001-feat-synthesize-verb-plan.md:
- Around line 94-115: The markdown code fence in the plan's numbered list is
missing a language tag which triggers MD040; update the opening fence fromto include a language (for example,text or ```md) so markdownlint
passes—modify the fenced block that contains the numbered steps (the "Parse
args, build providers..." through "Return result with synthesis as the answer")
to use a tagged fence.In
@docs/solutions/integration-issues/opencode-provider-integration.md:
- Around line 35-80: Two fenced code blocks in the docs are missing language
labels (the block starting with "refinery provider: opencode / refinery model:
kimi-for-coding/..." and the final examples block listing
"opencode/minimax-m2.5-free ... zai-coding-plan/glm-5"); update each opening
triple-backtick to include a language (e.g.,text) so they becometext ...unchanged. In `@docs/solutions/logic-errors/consensus-tiebreaking-and-winner-semantics.md`: - Line 17: The fenced code block containing the example lines starting with "R1: opus=8.8 ★, kimi=8.8" is missing a language token and triggers markdownlint MD040; update that fence to include a language (e.g., add "text" so the opening fence becomes ```text) so the block is explicitly marked and the linter error is resolved. In `@README.md`: - Line 200: Replace unlabeled fenced code blocks showing example CLI output with language-labeled fences (use ```text) so markdownlint MD040 is satisfied; specifically update the block that begins with the example output line "$ refinery converge ..." to start with "```text" instead of "```", and apply the same change to the other unlabeled example-output fences noted (the blocks near the other example outputs). Ensure only the fence markers are changed (no content modifications) so the examples render as plain text. In `@todos/003-verb-synthesize.md`: - Line 20: The doc entry for the flag `--synthesis-threshold` currently uses the placeholder text "default: threshold" which is ambiguous; update that line to show the concrete default value (e.g., "default: 0.5") or explicitly reference the canonical source constant or flag name (for example, DEFAULT_SYNTHESIS_THRESHOLD or SynthesisConfig.SYNTHESIS_THRESHOLD_DEFAULT) so operators see the exact default; change the text for `--synthesis-threshold` to either the numeric default or a clear pointer to the source where the default is defined.🪄 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:
5d7f57ae-e55c-4819-967e-5db02bed855e⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock📒 Files selected for processing (89)
.gitignoreCLAUDE.mdCargo.tomlREADME.mdcrates/refinery_cli/Cargo.tomlcrates/refinery_cli/src/commands/brainstorm.rscrates/refinery_cli/src/commands/common.rscrates/refinery_cli/src/commands/converge.rscrates/refinery_cli/src/commands/mod.rscrates/refinery_cli/src/commands/synthesize.rscrates/refinery_cli/src/main.rscrates/refinery_cli/src/progress.rscrates/refinery_core/Cargo.tomlcrates/refinery_core/src/brainstorm.rscrates/refinery_core/src/engine.rscrates/refinery_core/src/error.rscrates/refinery_core/src/lib.rscrates/refinery_core/src/phases/close.rscrates/refinery_core/src/phases/evaluate.rscrates/refinery_core/src/phases/mod.rscrates/refinery_core/src/phases/propose.rscrates/refinery_core/src/phases/refine.rscrates/refinery_core/src/progress.rscrates/refinery_core/src/prompts.rscrates/refinery_core/src/prompts/brainstorm.rscrates/refinery_core/src/prompts/mod.rscrates/refinery_core/src/prompts/synthesize.rscrates/refinery_core/src/scoring.rscrates/refinery_core/src/strategy.rscrates/refinery_core/src/testing.rscrates/refinery_core/src/todos/006-file-budget-error-message.mdcrates/refinery_core/src/types.rscrates/refinery_providers/src/lib.rscrates/refinery_providers/src/process.rscrates/tundish_cli/Cargo.tomlcrates/tundish_cli/src/main.rscrates/tundish_core/Cargo.tomlcrates/tundish_core/src/error.rscrates/tundish_core/src/lib.rscrates/tundish_core/src/progress.rscrates/tundish_core/src/types.rscrates/tundish_core/src/util.rscrates/tundish_providers/Cargo.tomlcrates/tundish_providers/src/claude.rscrates/tundish_providers/src/codex.rscrates/tundish_providers/src/credential.rscrates/tundish_providers/src/gemini.rscrates/tundish_providers/src/lib.rscrates/tundish_providers/src/opencode.rscrates/tundish_providers/src/process.rscrates/tundish_providers/src/tools.rsdocs/HANDOFF.mddocs/brainstorms/2026-03-13-provider-model-syntax-brainstorm.mddocs/brainstorms/2026-03-17-cli-subcommand-verbs-brainstorm.mddocs/brainstorms/2026-03-18-synthesize-verb-requirements.mddocs/brainstorms/2026-03-30-brainstorm-verb-requirements.mddocs/plans/2026-03-13-refactor-provider-model-syntax-plan.mddocs/plans/2026-03-13-refactor-remove-refine-phase-plan.mddocs/plans/2026-03-14-feat-indicatif-multi-spinner-comfy-table-plan.mddocs/plans/2026-03-17-refactor-cli-subcommand-converge-plan.mddocs/plans/2026-03-18-001-feat-synthesize-verb-plan.mddocs/plans/2026-03-26-001-fix-pr26-review-comment-triage-plan.mddocs/plans/2026-03-31-001-feat-brainstorm-verb-plan.mddocs/plans/2026-04-01-001-test-brainstorm-verb-plan.mddocs/plans/2026-04-01-002-fix-brainstorm-review-comments-plan.mddocs/solutions/debugging-methodology/multi-model-consultation.mddocs/solutions/debugging-methodology/signal-testing-pitfalls.mddocs/solutions/debugging-methodology/test-before-declaring-done.mddocs/solutions/integration-issues/cli-provider-subprocess-isolation.mddocs/solutions/integration-issues/opencode-provider-integration.mddocs/solutions/integration-issues/provider-cli-quirks-and-fixes.mddocs/solutions/integration-issues/tui-progress-display-lessons.mddocs/solutions/logic-errors/consensus-tiebreaking-and-winner-semantics.mddocs/solutions/logic-errors/echo-provider-queue-ordering-in-multi-model-tests.mddocs/solutions/runtime-errors/ctrl-c-sigint-terminal-isolation.mddocs/solutions/runtime-errors/streaming-error-detection-and-model-validation.mdtodos/001-setsid-process-groups.mdtodos/002-cli-subcommand-converge.mdtodos/003-verb-synthesize.mdtodos/004-verb-brainstorm.mdtodos/005-verb-mine.mdtodos/010-tty-gate-ansi-escape-codes.mdtodos/011-verb-evolve.mdtodos/013-brainstorm-strategy-benchmarks.mdtodos/014-brainstorm-r4-wording-clarification.mdtodos/015-brainstorm-todo-upvote-terminology.mdtodos/016-silent-json-serialization-failure.mdtodos/017-brainstorm-p3-nitpicks.mdtodos/018-brainstorm-divergence-expansion.md💤 Files with no reviewable changes (5)
- crates/refinery_core/src/phases/refine.rs
- crates/refinery_providers/src/lib.rs
- crates/refinery_providers/src/process.rs
- crates/refinery_core/src/phases/mod.rs
- crates/refinery_core/src/prompts.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab10b42191
ℹ️ 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".
| match result { | ||
| Ok((from, to, Ok(Ok(response)))) => { | ||
| eval_count += 1; | ||
| let parsed: serde_json::Value = serde_json::from_str(&response).unwrap_or_default(); |
There was a problem hiding this comment.
When a schema-less provider such as opencode evaluates syntheses, synthesize_evaluate_prompt() explicitly asks for a JSON block and shows a fenced ```json example, but this parser only accepts raw JSON. A fenced response that the normal evaluate phase would accept via prompts::extract_json becomes `Null`, every score is skipped, and runs with those evaluators can incorrectly end as `No synthesis evaluations completed`; unwrap the fenced JSON before deserializing here.
Useful? React with 👍 / 👎.
| // ── Evaluate ──────────────────────────────────────────────────── | ||
|
|
||
| // Single model: skip evaluation (no self-eval). | ||
| if round_proposals.len() == 1 { |
There was a problem hiding this comment.
When exactly one model produces a proposal in a brainstorm round (including normal single-model brainstorms, or rounds where all peers fail), this branch skips evaluation and then continues before appending anything to score_histories. The next round is therefore prompted with empty history again, so --max-rounds repeats first-attempt prompts instead of letting the model iterate on its prior answer/score-only feedback; record a history entry or short-circuit before continuing.
Useful? React with 👍 / 👎.
| let qualifying: Vec<_> = outcome | ||
| .all_answers | ||
| .iter() | ||
| .filter(|a| a.mean_score >= synthesis_threshold) | ||
| .collect(); |
There was a problem hiding this comment.
For refinery synthesize with one model, Engine short-circuits without evaluations, so the sole all_answers entry has mean_score == 0.0. Since the CLI enforces --synthesis-threshold >= 1.0 and defaults it to 8.0, this filter always removes the only answer and returns no_qualifying_answers, making the later single-synthesis path unreachable; include the single answer or bypass the threshold when there are no evaluation scores.
Useful? React with 👍 / 👎.
El-Fitz
dismissed
coderabbitai[bot]’s stale review
Addressed in ab10b42; CI and CodeRabbit status are passing.
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Add score-only brainstorm iteration with controversial panel selection, synthesize/converge CLI restructuring, tundish provider extraction, docs/plans/todos, and captured divergence-expansion follow-ups for prompt reframing and Open Collider-style domain collisions.
El-Fitz
force-pushed
the
feat/brainstorm-verb
branch
from
ab10b42 to
d8b6a66
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8b6a66d10
ℹ️ 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 timeout = Duration::from_secs(shared.timeout); | ||
| let idle_timeout = Duration::from_secs(shared.idle_timeout); | ||
|
|
||
| let hidden = shared.verbose || shared.debug || !std::io::stderr().is_terminal(); |
There was a problem hiding this comment.
SharedArgs documents --verbose as “Show per-round progress”, but this condition marks the display as hidden whenever verbose is true, which disables both the spinner/tick output and progress callbacks on interactive TTY runs. In practice, --verbose currently suppresses the progress UI instead of enabling it across converge/synthesize/brainstorm paths that use build_providers, so users lose the feature they explicitly requested.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
Note: direct push to main is blocked by repository rules; this PR should go through the merge queue.