Skip to content

fix(quorum): backport CLI fallback to probe-quorum-slots#180

Merged
jobordu merged 1 commit into
mainfrom
fix/probe-quorum-slots-cli-fallback
May 15, 2026
Merged

fix(quorum): backport CLI fallback to probe-quorum-slots#180
jobordu merged 1 commit into
mainfrom
fix/probe-quorum-slots-cli-fallback

Conversation

@jobordu
Copy link
Copy Markdown

@jobordu jobordu commented May 14, 2026

Summary

Why

`provider.cli` is null for all 5 sub slots (codex-1, gemini-1, opencode-1, copilot-1, claude-1) in the canonical `~/.claude/nf/bin/providers.json`. The hot path (`call-quorum-slot.cjs`) handles this via the three-tier fallback added in #164, but the diagnostic probe script was missing the same fix. Calling it directly would crash before reaching any slot.

Surfaced by a `/nf:quorum` self-test run where codex-1 caught the gap (cited `bin/probe-quorum-slots.cjs:27,67`). Quorum reached consensus that the hot-path was fine but the probe tool needed the same backport.

Test plan

  • `node bin/probe-quorum-slots.cjs --slots codex-1,gemini-1,opencode-1,copilot-1,claude-1 --timeout 8000` returns valid JSON (3 healthy, 2 timeouts at 8s — gemini-1 and claude-1 are real LLM calls; not a regression).
  • `npm run lint:isolation` → passes
  • `node --test test/quorum-preflight-probe.test.cjs` → 9/9 pass
  • CI: run full `test:ci` suite

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Graceful failure when no executable can be resolved for a provider—slots now report failures instead of attempting to spawn invalid commands.
    • Robust provider-file loading: skips empty provider lists and continues on parse/read errors.
  • Improvements

    • Expanded automatic discovery paths for provider configuration files.
    • Caches resolved executables and improves fallback logic when CLI specs are missing.

Review Change Stack

probe-quorum-slots.cjs would crash with `spawn: file argument must be of
type string. Received null` when invoked because `provider.cli` is null
for all 5 sub slots (codex-1, gemini-1, opencode-1, copilot-1, claude-1)
in the canonical providers.json. PR #164 fixed this in call-quorum-slot
via a `resolvedCli || cli || mainTool` fallback chain — this patch
applies the same fix to the probe script.

Also backports the findProviders() skip-empty + canonical-first ordering
from PR #165 so the repo's empty `bin/providers.json` skeleton doesn't
short-circuit the search before the real installed providers.json is
found.

Surfaced by a /nf:quorum self-test run where codex-1 caught the gap;
6/6 voters reached consensus that hot-path was fine but the probe tool
needed the same backport.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 14, 2026 07:48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

Walkthrough

bin/probe-quorum-slots.cjs now expands provider file discovery paths to include ~/.claude locations, improves provider file loading with existence checks and empty-provider-list handling, and implements CLI resolution for subprocess providers using resolveCli to derive spawn targets from provider.mainTool when provider.cli is omitted.

Changes

Quorum slot probing with CLI resolution

Layer / File(s) Summary
Provider file discovery and loading
bin/probe-quorum-slots.cjs
Adds resolveCli import, extends findProviders() search paths to include additional ~/.claude locations, and updates provider-file loading to check existence first, parse { providers }, skip empty provider arrays, and continue on errors.
CLI resolution for subprocess probing
bin/probe-quorum-slots.cjs
In probeSlot(), treats provider.cli as optional, derives provider.resolvedCli from provider.mainTool via resolveCli when needed, selects a non-empty spawn target, and returns a structured failure result when no resolvable spawn target exists; uses spawnTarget for the spawn call.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • nForma-AI/nForma#161: Implements the same resolveCli fallback pattern for subprocess CLI resolution in unified-mcp-server.mjs to avoid null spawn targets.
  • nForma-AI/nForma#165: Updates provider discovery to prefer ~/.claude/nf/bin/providers.json and improves loading to skip empty/missing provider lists.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: backporting CLI fallback logic to the probe-quorum-slots script to fix null reference crashes.
Description check ✅ Passed The description covers what was changed, why it was needed, and includes test results, following the template's core requirements even though not all checkbox items are completed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/probe-quorum-slots-cli-fallback

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jobordu
Copy link
Copy Markdown
Author

jobordu commented May 15, 2026

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
bin/probe-quorum-slots.cjs (1)

35-38: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the canonical providers file first.

Line 38 prepends path.dirname(serverScript)/providers.json, so a non-empty sibling file can still override ~/.claude/nf/bin/providers.json. That undermines the canonical-first lookup this backport is trying to restore and can make the probe read a stale repo/local config instead of the live quorum config.

Suggested fix
   try {
     const claudeJson = JSON.parse(fs.readFileSync(path.join(os.homedir(), '.claude.json'), 'utf8'));
     const u1args = claudeJson?.mcpServers?.['unified-1']?.args ?? [];
     const serverScript = u1args.find(a => typeof a === 'string' && a.endsWith('unified-mcp-server.mjs'));
-    if (serverScript) searchPaths.unshift(path.join(path.dirname(serverScript), 'providers.json'));
+    if (serverScript) {
+      const siblingProviders = path.join(path.dirname(serverScript), 'providers.json');
+      if (!searchPaths.includes(siblingProviders)) searchPaths.splice(1, 0, siblingProviders);
+    }
   } catch (_) {}
🤖 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/probe-quorum-slots.cjs` around lines 35 - 38, The code currently prepends
the sibling providers.json (found via serverScript) into searchPaths which
allows that file to override the canonical ~/.claude/nf/bin/providers.json;
change the logic so the sibling providers.json is appended (use
searchPaths.push(path.join(path.dirname(serverScript), 'providers.json')) or
otherwise insert it after the canonical entry) so the canonical providers file
remains first; update the block that reads claudeJson, computes u1args and
serverScript and replace the unshift call on searchPaths with a push (or
equivalent insertion after the canonical path) to preserve canonical-first
lookup.
🤖 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.

Outside diff comments:
In `@bin/probe-quorum-slots.cjs`:
- Around line 35-38: The code currently prepends the sibling providers.json
(found via serverScript) into searchPaths which allows that file to override the
canonical ~/.claude/nf/bin/providers.json; change the logic so the sibling
providers.json is appended (use
searchPaths.push(path.join(path.dirname(serverScript), 'providers.json')) or
otherwise insert it after the canonical entry) so the canonical providers file
remains first; update the block that reads claudeJson, computes u1args and
serverScript and replace the unshift call on searchPaths with a push (or
equivalent insertion after the canonical path) to preserve canonical-first
lookup.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ef3456b1-2454-4f08-87a9-abee50183f3f

📥 Commits

Reviewing files that changed from the base of the PR and between ec1fe57 and b1540c0.

📒 Files selected for processing (1)
  • bin/probe-quorum-slots.cjs

@jobordu jobordu merged commit d7f9e91 into main May 15, 2026
15 checks passed
@jobordu jobordu mentioned this pull request May 21, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants