feat(llm-council): add --sequential flag for serial model calls - #95
Open
23r2efewvcs wants to merge 8 commits into
Open
feat(llm-council): add --sequential flag for serial model calls#9523r2efewvcs wants to merge 8 commits into
23r2efewvcs wants to merge 8 commits into
Conversation
Hardcoded max_tokens: 4000 is too small for reasoning models (Nemotron Ultra, GLM-5.2, DeepSeek-V4, Claude thinking) which need 8K-32K output tokens for their reasoning chain or they emit empty content. Introduce RUN_OPTS struct + --max-tokens flag (default 4000, preserves current behavior). Both callOpenAICompat and callAnthropic read from RUN_OPTS.max_tokens. Fixes rohitg00#88.
The HTTP request timeout is hardcoded at 120s in postJSON(). This is too short for slow upstream endpoints (NVIDIA NIM reasoning models, OpenRouter under load), causing ETIMEDOUT / 'council request timeout' failures with no way to extend. Introduce --timeout N flag (default 120000, preserves current behavior) sourced into RUN_OPTS.timeout_ms. Documented in usage(). Depends on rohitg00#89 (--max-tokens, which introduces RUN_OPTS struct). Fixes rohitg00#90.
… backoff Single transient 429 or 5xx kills the entire session. Free-tier endpoints (NVIDIA NIM, OpenRouter :free) are especially prone. Add --max-retries N flag (default 1, preserves current one-shot behavior). Both callOpenAICompat and callAnthropic now: - catch connection-level errors (ETIMEDOUT, ECONNRESET) and retry - retry on 429/5xx with exponential backoff (2s, 4s, 8s...) - fail fast after retries exhausted with clear [ERROR ...] message Depends on rohitg00#91 (--timeout, which introduces RUN_OPTS). Fixes rohitg00#92.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe council CLI adds configurable token limits, request timeouts, retry counts, and sequential execution. Provider requests use shared settings and retry handling. Both model phases support parallel or sequential execution. ChangesCouncil runtime controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🔵 Low · up to The change adds serial model calls, while numeric command-line options can still accept malformed or unsafe values that may lead to unintended retry behavior. This is a bounded risk and the PR is mergeable with explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant CLI
participant runCalls
participant ProviderCall
participant ModelEndpoint
CLI->>runCalls: Select parallel or sequential execution
runCalls->>ProviderCall: Execute model request
ProviderCall->>ModelEndpoint: Send request with timeout and token settings
ModelEndpoint-->>ProviderCall: Return response or retryable error
ProviderCall->>ModelEndpoint: Retry with exponential backoff
ProviderCall-->>runCalls: Return settled result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@skills/llm-council/scripts/council.js`:
- Around line 336-341: Update the help-text options block in council.js so
--max-retries documents retries for connection errors as well as 429/5xx
responses. Align the command synopsis in SKILL.md with the runtime-supported
controls by adding --sequential, --max-tokens, --timeout, and --max-retries,
keeping descriptions and defaults consistent with council.js.
- Around line 212-214: Validate the numeric CLI options in the argument-handling
block before assigning to RUN_OPTS: require max-tokens and timeout to be
positive integers, and max-retries to be a non-negative integer. Reject
non-integer or trailing-character input instead of relying on parseInt’s partial
parsing, and only set RUN_OPTS.max_tokens, RUN_OPTS.timeout_ms, or
RUN_OPTS.max_retries after validation succeeds.
- Around line 219-220: The runCalls function passes arrow functions directly to
Promise.allSettled without invoking them, causing Promise.allSettled to resolve
with function objects instead of response promises. This breaks downstream code
like settledToEntry that expects settled.value to contain response data with a
content property. Invoke each callable function (for example, by mapping over
callables and calling each one) before passing the resulting promises to
Promise.allSettled.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 453b2de6-5dde-4a6c-910f-7fdcf624356f
📒 Files selected for processing (1)
skills/llm-council/scripts/council.js
Comment on lines
+212
to
+214
| if (args['max-tokens']) RUN_OPTS.max_tokens = parseInt(args['max-tokens'], 10); | ||
| if (args.timeout) RUN_OPTS.timeout_ms = parseInt(args.timeout, 10); | ||
| if (args['max-retries']) RUN_OPTS.max_retries = parseInt(args['max-retries'], 10); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 "parseInt|Number\\.isSafeInteger|max-retries|timeout_ms|max_tokens" \
skills/llm-council/scripts/council.jsRepository: rohitg00/pro-workflow
Length of output: 4400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the retry loop and CLI argument parsing around the reported options.
sed -n '85,155p' skills/llm-council/scripts/council.js
printf '\n--- parseArgs context ---\n'
sed -n '180,210p' skills/llm-council/scripts/council.js
printf '\n--- numeric edge-case behavior ---\n'
node - <<'JS'
const cases = [undefined, 'abc', '1.5', '-1', '0', '1', ' 1 ', '1abc'];
for (const raw of cases) {
const parsed = raw === undefined ? undefined : parseInt(raw, 10);
let attempt = 0;
const limit = parsed;
let count = 0;
// Simple analog: while (true) { attempt++; if (attempt >= limit) break }
while (true) {
attempt++;
if (limit !== undefined && attempt >= limit) break;
if (++count >= 4) break;
}
const exitsAt = !Number.isSafeInteger(limit) || limit <= 0 ? 'not limited by NaN' : limit;
console.log(JSON.stringify({ raw, parsed, isSafeIntAndPositive: Number.isSafeInteger(limit) && limit > 0, loopIterationsUntilExitOrBounded: count, attemptAfterLoop: attempts => attempt, limit: parsed }));
}
console.log(JSON.stringify({
isNaNParsed: isNaN(parseInt('abc', 10)),
comparisonNaN: parseInt('abc', 10) >= 1,
comparisonNegative: parseInt('-1', 10) >= 1,
comparisonFractionParsed: parseInt('1.5', 10) >= 1
}));
JSRepository: rohitg00/pro-workflow
Length of output: 5010
Validate numeric command-line options before setting them.
parseInt only reads the leading integer, so --max-retries=1abc still sets RUN_OPTS.max_retries to 1, and invalid --max-retries values can let retryable 429/5xx or connection errors retry indefinitely. Use a safe integer validator and reject non-integer input. Require --max-tokens/--timeout to be positive, and require --max-retries to be non-negative.
🤖 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 `@skills/llm-council/scripts/council.js` around lines 212 - 214, Validate the
numeric CLI options in the argument-handling block before assigning to RUN_OPTS:
require max-tokens and timeout to be positive integers, and max-retries to be a
non-negative integer. Reject non-integer or trailing-character input instead of
relying on parseInt’s partial parsing, and only set RUN_OPTS.max_tokens,
RUN_OPTS.timeout_ms, or RUN_OPTS.max_retries after validation succeeds.
23r2efewvcs
added a commit
to 23r2efewvcs/pro-workflow
that referenced
this pull request
Jul 31, 2026
- Fix runCalls: invoke callables before Promise.allSettled - Add parseIntSafe: validate CLI options are positive integers - Update SKILL.md with --max-tokens, --timeout, --max-retries, --sequential Addresses CodeRabbit comments on PR rohitg00#95.
- Add parseIntSafe: validate --max-tokens, --timeout, --max-retries are positive integers, reject NaN/negative/non-integer - Update SKILL.md with new CLI flags and runtime options table Addresses CodeRabbit comments on PR rohitg00#93.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/llm-council/SKILL.md`:
- Line 58: Update the “Three phases” descriptions for Phase 1 and Phase 2 to
state that their model calls run in parallel by default, while documenting that
the --sequential option overrides this behavior for those phases and runs them
sequentially.
- Line 46: The multiline command in the code block is missing a shell
continuation character, causing the command to break when copied and executed.
Add a backslash (`\`) at the end of the line immediately preceding the options
line containing `[--max-tokens N] [--timeout ms] [--max-retries N]
[--sequential]` to properly continue the command on the next line. This ensures
the entire command executes as a single unit in the shell.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 880515cf-cacd-4609-a64d-57719d7f721f
📒 Files selected for processing (2)
skills/llm-council/SKILL.mdskills/llm-council/scripts/council.js
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/llm-council/scripts/council.js
Both callOpenAICompat and callAnthropic duplicated the same retry/backoff
loop. Extract a shared postJSONWithRetry(url, payload, headers) helper that
returns { res } or { error }, preserving max_retries, 429/5xx handling,
connection-error handling, and exponential backoff (2s, 4s, 8s...).
Addresses CodeRabbit comment on PR rohitg00#93.
Phase 1 and Phase 2 run all model calls concurrently via Promise.allSettled. On free-tier endpoints with strict concurrent-request limits (NVIDIA NIM free: 1 concurrent per API key, OpenRouter :free under load), this causes ETIMEDOUT or 429 on the 2nd/3rd call. Add --sequential flag (default: parallel, preserves current behavior). When set, calls run one at a time via a runCalls() helper that serializes execution. Slower but avoids concurrent-request rejections. Depends on rohitg00#93 (--max-retries, which introduces the retry loop). Fixes rohitg00#94.
- Fix runCalls: invoke callables before Promise.allSettled - Add parseIntSafe: validate CLI options are positive integers - Update SKILL.md with --max-tokens, --timeout, --max-retries, --sequential Addresses CodeRabbit comments on PR rohitg00#95.
- Add shell continuation backslash to multiline command in SKILL.md - Clarify phases 1 and 2 run in parallel by default; --sequential overrides - Document that --max-retries covers connection errors in help text Addresses CodeRabbit comments on PR rohitg00#95.
23r2efewvcs
force-pushed
the
feat/llm-council-sequential
branch
from
August 13, 2026 11:34
9fd9fd6 to
f9fa450
Compare
23r2efewvcs
added a commit
to 23r2efewvcs/pro-workflow
that referenced
this pull request
Aug 13, 2026
Modern reasoning models (OpenAI o1/o3, DeepSeek R1/V4, Claude thinking) accept a reasoning_effort parameter that controls compute budget. Without it, models either waste tokens on simple queries or truncate reasoning on complex ones. Add --reasoning-effort low|medium|high flag (default: not set, preserves current behavior). When set: - OpenAI-compat: adds reasoning.effort to payload - Anthropic: adds thinking.budget_tokens (derived from effort level) Depends on rohitg00#95 (--sequential). Fixes rohitg00#96.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #93.
Fixes #94.
What
Phase 1 and Phase 2 run all model calls concurrently via
Promise.allSettled. On free-tier endpoints with strict concurrent-request limits (NVIDIA NIM free: 1 concurrent per API key, OpenRouter:freeunder load), this causes ETIMEDOUT or 429 on the 2nd/3rd call.Change
sequential: falsetoRUN_OPTSrunCalls()helper that choosesPromise.allSettled(parallel) or serialfor...ofloopPromise.allSettledin Phase 1 and Phase 2 withrunCalls--sequentialCLI flag incmdRunusage()After this PR
Slower (serial) but avoids concurrent-request rejections on free endpoints. Diff: 20 +, 5 -.
Summary by CodeRabbit