harden(quorum): reliability fixes P1–P5 (stall false-kill, degraded-panel gate, quota cooldown, deep-probe gate, config validator) - #293
Open
jobordu wants to merge 7 commits into
Open
Conversation
![open-swe[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
The <500-byte adaptive idle window false-killed slow-but-healthy slots: a model
that emitted a short CLI preamble (e.g. 202-byte framing) then paused >30s while
generating was killed as STALL, because total output stayed under the 500-byte
threshold so the idle window was tightened to 30s mid-generation. Observed live on
claude-z-ai / claude-minimax ("202 bytes then 30s silence -> killed"), which
collapsed a 7-slot quorum to 1 across 6 rounds.
Fix: STALL now means "produced no FIRST byte", not "still thinking". A dedicated
first-byte timer fires only if zero bytes arrive by STALL_TIMEOUT_MS (genuine
dead-on-arrival hang); once ANY byte arrives it is cleared and the normal per-chunk
idle window governs, so slow streamers are not penalised. Also clamp the initial
idle/hard timers to TIMEOUT_MAX (Node fires >2^31-1ms timers immediately, which
would silently disable them) — previously only the stall window was clamped.
Empirically validated: widening the stall window on the two affected slots was the
only change that let them complete a heavy review prompt, confirming the diagnosis.
All 40 existing call-quorum-slot timeout/latency/infra/retry tests pass.
Follow-up: add a mock-CLI integration harness to time-test runSubprocess (also
needed for the P1/P3 "quota-dead slot blocked before review" test).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The "available < max_quorum_size → BLOCK unless --force-quorum" decision lived only
in quorum.md prose (LLM-interpreted), so a panel that collapsed to 1/7 could run
silent reduced rounds with no deterministic gate. This session did exactly that —
6 rounds at 1/7.
Add computeQuorumGate(availableCount, maxSize, forceQuorum) — a pure, exported,
tested function — and emit its result on the preflight --all/--probe output:
{ available_count, quorum_met, degraded, blocked, waiver_required|waiver_used,
gate_reason }. --force-quorum records an explicit waiver so the override is
machine-visible. Invariant: when `blocked` is true, no downstream path may dispatch
without a recorded waiver. `degraded` (1 <= available < max) is the signal P1's
deep-probe gate will auto-enable on.
7 new unit tests (incl. the 1-of-7 production shape); existing preflight
roster/probe/dedup suites still green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A quota-dead slot (e.g. antigravity-1: Google 429 "Resets in 32h54m49s") was re-probed and re-failed every 30 minutes because the failure TTL is far shorter than a rolling daily quota. Now the reset window is parsed from the error text and used as the cooldown, so the slot is held out until it actually recovers. - parseQuotaResetMs(msg): parses "Resets in 32h54m49s", "resets in 1h30m", "try again in 30 minutes", "in 2 hours", etc. → ms (clamped [0, 48h]); null if absent. - writeFailureLog: for QUOTA failures, stamp cooldown_until = now + parsed window; GC now preserves records whose cooldown_until is still in the future (else the long cooldown would be lost after the 60-min GC). - probeInferenceHistory: a record with a future cooldown_until keeps the slot unavailable until the real reset (surfaced as "cooling down ~Nmin"), with a `retired` flag for QUOTA holds. Non-quota blips never set cooldown_until, so a one-off failure can't retire a slot (addresses the reviewer's retirement-criterion concern). 6 new parser unit tests; preflight-probe / call-quorum infra/retry/stall / P3-gate suites all still green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was no schema gate on providers.json: a subprocess slot with no resolvable
spawn target, or an http slot missing baseUrl/apiKeyEnv, surfaced only as a spawn
crash at dispatch (spawn(null) took the whole quorum offline). A slot with no
deep_probe (e.g. antigravity-1) failed opaquely instead of being inference-gated.
Add validateProviders(providers) — pure, exported, tested — returning
{ ok, errors, warnings }: errors for unspawnable subprocess slots, http slots
missing baseUrl/apiKeyEnv, missing type, and duplicate names; warnings for inference
slots lacking a deep_probe. Wired as non-fatal diagnostics on the preflight --all path
so config drift is caught up front.
7 new unit tests (incl. the antigravity-1 no-deep_probe shape); preflight
gate/probe/roster/dedup suites still green.
NOTE (operational, not code): reconciling the drifted providers.json copies
(repo scaffold vs ~/.claude/nf-bin vs .claude/nf/bin) and dropping the quota-dead
antigravity-1 from nf.json quorum_active are user-config actions left to the operator;
P4's cooldown already holds antigravity-1 out until its quota resets.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…arness) L1 (--version) and L2 (/models) can't distinguish a quota/auth-dead slot from a healthy one — L2 even treats HTTP 401/403 as reachable — so a dead slot passes preflight and only fails during the real review (this session's failure). P1 adds a deep inference probe to catch it. Landed here (pure, tested, regression-safe): - --deep flag; shouldRunDeepProbe() auto-enables the gate when the panel is degraded (P3's `degraded`), budget permitting — happy path untouched (default off). - classifyDeepProbeResult(): downgrades a slot ONLY on a FAST explicit auth/quota signal (the class L1/L2 miss). A TIMEOUT is INCONCLUSIVE and never downgrades, so a slow-but-healthy slot (the P2 failure mode) cannot be re-killed by the deep probe; ambiguous non-error output is assumed alive (never false-kill on ambiguity). Deliberately NOT wired: the live subprocess spawn is not inserted into the main() hot path yet. An untested spawn on the path that gates EVERY quorum is the single change that could take the whole system offline, so it lands with a mock-CLI integration harness in a focused follow-up (the same harness that will time-test P2's runSubprocess and the reviewers' "quota-dead slot blocked before review" e2e case). 10 new policy unit tests; full preflight suite (gate/validate/probe/roster/dedup) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-CLI harness Completes P1 — the deep inference gate is now wired into preflight and exercised by a real subprocess harness (the deferred piece from the prior P1 commit). - deepProbeSlot(provider, budgetMs): spawns the provider's deep_probe prompt through the real CLI and classifies via classifyDeepProbeResult. Regression-safe: spawn/args errors and TIMEOUTS resolve ok:true (SKIP/INCONCLUSIVE) — it can ONLY downgrade on a fast explicit auth/quota signal, never on slowness (can't recreate the P2 false-kill). - Wired into `--all --probe`: after L1/L2/L3, if shouldRunDeepProbe() it deep-probes the available slots, moves any downgraded (auth/quota-dead) to unavailable (layer4), then computes the P3 gate on the post-deep roster — so a quota-dead slot that L1/L2 passed now blocks BEFORE any review runs. - CRITICAL fix vs the naive wiring: auto-enable on `degraded` requires an EXPLICIT budget (--budget-ms) that covers a probe. The cheap budget-less `--all --probe` liveness path must never pay deep-probe latency (a naive auto-enable made it spawnSync-ETIMEDOUT and broke its <8s budget). Real quorum dispatch passes --budget-ms, so it still auto-verifies a reduced panel; the fast probe path stays fast. Harness: bin/__deep-probe-mock.cjs (fake CLI: probe_ok/quota/auth/slow_ok/hang) drives 6 live integration tests incl. the reviewers' key E2E — a quota-dead slot is downgraded by the deep probe → the gate blocks a 2/3 panel before review. Slow-preamble-then-pause passes; hang→timeout is inconclusive (P2-safe). 110 tests green across the full call-quorum-slot + quorum-preflight suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds quota-reset cooldown tracking and first-byte stall-timer hardening to the subprocess dispatcher in call-quorum-slot.cjs, and introduces a deep-probe based degraded-quorum gating mechanism in quorum-preflight.cjs, including new pure helpers, providers.json validation, wiring into the --all flow, a mock CLI fixture, and accompanying unit/integration tests. ChangesQuota Cooldown and Stall-Detection Hardening
Estimated code review effort: 3 (Moderate) | ~30 minutes Deep-Probe Degraded-Quorum Gating
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as quorum-preflight --all
participant Validator as validateProviders
participant Prober as deepProbeSlot
participant MockCLI as provider CLI (mock)
participant Gate as computeQuorumGate
CLI->>Validator: validateProviders(providers)
Validator-->>CLI: errors, warnings
CLI->>Prober: deepProbeSlot(provider, budgetMs) for each available slot
Prober->>MockCLI: spawn with resolved args, timeout
MockCLI-->>Prober: stdout/stderr output or timeout
Prober-->>CLI: classification (OK / QUOTA / AUTH / INCONCLUSIVE / SKIP)
CLI->>CLI: downgrade slots with ok === false
CLI->>Gate: computeQuorumGate(availableCount, maxSize, forceQuorum)
Gate-->>CLI: blocked, waiver_required, waiver_used, gate_reason
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bin/call-quorum-slot.cjs (1)
264-270: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winParse the quota reset from ANSI-stripped text, not
rawPattern.
patternis ANSI-stripped, butparseQuotaResetMs(rawPattern)receives the raw text with escape codes intact. SGR sequences (e.g.\x1b[5m) end in<digits>m, which the parser can misread as a duration, and an ANSI code embedded inside the real reset value (Resets in \x1b[1m32h…) can also defeat theresets?\s+in\s+match. Strip first for reliable extraction.♻️ Proposed change
- const rawPattern = (stderrText && stderrText.length > 0) ? stderrText : errorMsg; - const pattern = rawPattern.replace(/\x1b\[[0-9;]*m/g, '').slice(0, 200); + const rawPattern = (stderrText && stderrText.length > 0) ? stderrText : errorMsg; + const cleaned = rawPattern.replace(/\x1b\[[0-9;]*m/g, ''); + const pattern = cleaned.slice(0, 200); @@ - const quotaResetMs = error_type === 'QUOTA' ? parseQuotaResetMs(rawPattern) : null; + const quotaResetMs = error_type === 'QUOTA' ? parseQuotaResetMs(cleaned) : null;🤖 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 `@bin/call-quorum-slot.cjs` around lines 264 - 270, The QUOTA reset parser is being fed the raw stderr text with ANSI escape codes still present, which can corrupt duration matching in the quota handling flow. In call-quorum-slot.cjs, update the QUOTA branch so parseQuotaResetMs receives the ANSI-stripped text (reuse the cleaned string used to build pattern, or strip before parsing) rather than rawPattern. Keep the existing quotaResetMs logic in the same area, but ensure the value passed into parseQuotaResetMs is sanitized first so reset windows are extracted reliably.
🤖 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 `@bin/quorum-preflight.cjs`:
- Around line 608-612: The provider validation path in quorum-preflight only
logs `_val.errors` and still proceeds into `buildTeam()` and `probeHealth()`,
which can produce incorrect `health[p.name]` and `available_count` results when
duplicate or invalid providers exist. Update the `validateProviders(providers)`
handling to stop the `--all` flow whenever `errors.length > 0`, or return a
machine-readable config-failure result instead of continuing after stderr
logging. Keep the fix localized around `validateProviders`, `buildTeam`, and
`probeHealth` so invalid configs cannot reach fan-out.
- Around line 389-392: The deep-gate helper is skipping every provider slot that
lacks a spawn target or args template, which causes HTTP providers to never
enter the new downgrade flow. Update the gate logic in the helper that checks
resolveSpawnTarget/provider.cli and resolveArgsTemplate so HTTP entries are
either handled by a dedicated deep-probe path or excluded consistently from this
gate; then make sure the caller in validateProviders uses the same contract when
counting available slots so HTTP backups are not classified as SKIP by default.
---
Nitpick comments:
In `@bin/call-quorum-slot.cjs`:
- Around line 264-270: The QUOTA reset parser is being fed the raw stderr text
with ANSI escape codes still present, which can corrupt duration matching in the
quota handling flow. In call-quorum-slot.cjs, update the QUOTA branch so
parseQuotaResetMs receives the ANSI-stripped text (reuse the cleaned string used
to build pattern, or strip before parsing) rather than rawPattern. Keep the
existing quotaResetMs logic in the same area, but ensure the value passed into
parseQuotaResetMs is sanitized first so reset windows are extracted reliably.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88a42356-905b-44a6-ba2d-14758c45e7ab
📒 Files selected for processing (8)
bin/__deep-probe-mock.cjsbin/call-quorum-slot-quota-reset.test.cjsbin/call-quorum-slot.cjsbin/quorum-preflight-deep-integration.test.cjsbin/quorum-preflight-deep.test.cjsbin/quorum-preflight-gate.test.cjsbin/quorum-preflight-validate.test.cjsbin/quorum-preflight.cjs
Comment on lines
+389
to
+392
| const target = resolveSpawnTarget(provider) || provider.cli; | ||
| const tmpl = resolveArgsTemplate(provider); | ||
| if (!target || !Array.isArray(tmpl)) { | ||
| resolve({ ok: true, classification: 'SKIP', reason: 'no spawn target / args_template' }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
HTTP providers are always skipped by the deep gate.
At Lines 389-392, any slot without a spawn target or args_template becomes SKIP. The caller at Line 709 applies this helper to every available slot, while the final gate still counts HTTP backups in available_count. That leaves auth/quota-dead HTTP slots outside the new P1 downgrade path even though validateProviders() treats http entries as part of the deep_probe contract. Either add an HTTP deep-probe path or keep HTTP slots out of this gate/counting contract.
🤖 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 `@bin/quorum-preflight.cjs` around lines 389 - 392, The deep-gate helper is
skipping every provider slot that lacks a spawn target or args template, which
causes HTTP providers to never enter the new downgrade flow. Update the gate
logic in the helper that checks resolveSpawnTarget/provider.cli and
resolveArgsTemplate so HTTP entries are either handled by a dedicated deep-probe
path or excluded consistently from this gate; then make sure the caller in
validateProviders uses the same contract when counting available slots so HTTP
backups are not classified as SKIP by default.
Comment on lines
+608
to
+612
| // P5 — surface providers.json config problems (drift, unspawnable slots, missing | ||
| // deep_probe) as non-fatal diagnostics rather than letting them crash at fan-out. | ||
| const _val = validateProviders(providers); | ||
| for (const e of _val.errors) process.stderr.write(`[preflight] config ERROR: ${e}\n`); | ||
| for (const w of _val.warnings) process.stderr.write(`[preflight] config warn: ${w}\n`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Block --all on provider validation errors.
At Line 610, _val.errors is only logged and execution continues. Duplicate names then collapse in health[p.name] and new Map(activeProviders.map(...)), so the emitted JSON can report the wrong roster and available_count even though validation already knows the config is invalid. Stop before buildTeam() / probeHealth() when errors.length > 0, or return a machine-readable config-failure state instead of only writing to stderr.
🤖 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 `@bin/quorum-preflight.cjs` around lines 608 - 612, The provider validation
path in quorum-preflight only logs `_val.errors` and still proceeds into
`buildTeam()` and `probeHealth()`, which can produce incorrect `health[p.name]`
and `available_count` results when duplicate or invalid providers exist. Update
the `validateProviders(providers)` handling to stop the `--all` flow whenever
`errors.length > 0`, or return a machine-readable config-failure result instead
of continuing after stderr logging. Keep the fix localized around
`validateProviders`, `buildTeam`, and `probeHealth` so invalid configs cannot
reach fan-out.
… docs + F1 test Two-model adversarial assessment (claude-z-ai + claude-minimax) of the P1–P5 branch. Applying the agreed should-fixes; the rest are documented as accepted gaps. - F2 (should-fix): validateProviders now skips `active:false` slots — an intentionally disabled api-* HTTP slot legitimately omits baseUrl/apiKeyEnv and was emitting ERROR on every preflight (stderr flood). +2 tests (skip inactive; active-incomplete still errors). - N2 (harden): deepProbeSlot.finish() captures `child` locally and guards the kill, so a not-yet-assigned/exited child can never throw a ReferenceError out of the Promise.all fan-out. - F1 (documented + regression bar): a quota/auth-dead slot in a FULL panel (degraded=false, no --deep) is not deep-probed → dispatched, L3 catches it next round (self-heals). Added a test pinning this accepted behavior + the --deep escape hatch. Deep-probing every slot every run is too expensive to make the default. - F3 / N1 (documented): http slots bypass P1's CLI deep-probe (need an HTTP completion probe — out of scope); budgetMs is applied per-slot in the parallel fan-out. - F4 REFUTED, N5 REFUTED (validator requires BOTH cli+mainTool absent — mainTool-only is fine). Full preflight suite green (validate 9, deep 12, gate 7, deep-integration 7, probe 9). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Why
A live multi-model quorum run collapsed to 1 of 7 slots across 6 rounds. Diagnosis (via
/nf:mcp-repair+ live probes) showed the slots were not down — they were slow, mis-timed-out, or quota-dead with opaque errors. This PR hardens the five root causes. Plan reviewed by two independent models (claude-z-ai GO, claude-minimax GO-WITH-CHANGES; refinements folded in).What (6 commits, reviewer order)
call-quorum-slot.cjs). The<500-byteadaptive window tightened idle to 30s mid-generation, so a slow model that emitted a short preamble then paused >30s was killed asSTALL(the observed "202 bytes then 30s silence → killed"). Now a dedicated first-byte timer catches only genuine zero-output hangs; once any byte arrives the normal per-chunk idle window governs. Also clamps the initial idle/hard timers toTIMEOUT_MAX.quorum-preflight.cjs).computeQuorumGate()emits{ available_count, quorum_met, degraded, blocked, waiver_required|waiver_used, gate_reason }. The "available < max_quorum_size → BLOCK unless--force-quorum" decision was previously LLM-interpreted prose inquorum.md. Invariant: whenblocked, no dispatch without a recorded--force-quorumwaiver.call-quorum-slot.cjs+quorum-preflight.cjs).parseQuotaResetMs()reads the reset window ("Resets in 32h54m") and stampscooldown_until, so a quota-dead slot is held out to its real reset instead of re-failing every 30 min; GC preserves future-cooldown records. Non-quota blips never set a cooldown (no one-off retirement).quorum-preflight.cjs).validateProviders()errors on unspawnable subprocess slots / http slots missingbaseUrl+apiKeyEnv/ missing type / duplicate names; warns on inference slots lacking adeep_probe. Non-fatal diagnostics on--all.quorum-preflight.cjs). L1 (--version) and L2 (/models, which treats 401/403 as reachable) can't tell a quota/auth-dead slot from a healthy one.deepProbeSlot()runs the realdeep_probeand downgrades a slot only on a fast explicit auth/quota signal — a timeout/spawn-error is inconclusive and never downgrades (can't recreate the P2 false-kill). Auto-enables on a degraded panel only when an explicit--budget-mscovers it, so the cheap budget-less--all --probeliveness path stays fast.Safety / regression notes
spawnSync ETIMEDOUTand broke the<8sbudget — caught and fixed).Tests
110 tests green across the
call-quorum-slot+quorum-preflightsuites. New: quota-reset parser (6), degraded-panel gate (7, incl. the 1-of-7 production shape), config validator (7), deep-probe policy (11), and a mock-CLI live integration harness (__deep-probe-mock.cjs, 7) exercising the real spawn — including the reviewers' key E2E: a quota-dead slot is downgraded by the deep probe → the gate blocks a 2/3 panel before any review runs.Follow-ups (not in this PR)
runSubprocessvia the new mock-CLI harness (exportrunSubprocess).providers.jsoncopies; drop the quota-deadantigravity-1fromnf.jsonquorum_active(P4's cooldown already holds it out).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes