Repository navigation
fix(cli): forward setup --check and --non-interactive flags to provider delegation - #1343
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe setup command now forwards ChangesSetup flag forwarding
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI as handleSetup
participant Delegate as delegateToProviderSetup
participant Provider as OpenAI provider setup
CLI->>Delegate: Forward check and nonInteractive
Delegate->>Provider: Pass normalized setup flags
Provider-->>CLI: Complete setup without prompts when applicable
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
Fixes the CLI setup provider-delegation path so neurolink setup --provider <p> --check and --non-interactive are no longer silently dropped, preventing unintended interactive prompts (and failures/hangs) in non-TTY/scripted environments. This aligns the --provider flag form with the already-correct positional subcommand behavior (neurolink setup <p> ...) and adds regression coverage in the built-CLI subprocess test suite.
Changes:
- Extend
SetupArgsto includecheckandnonInteractive. - Forward
check/nonInteractivefromhandleSetup()intodelegateToProviderSetup(), and keep the"non-interactive"argv key in sync for provider handlers that read it. - Add CLI subprocess regression tests verifying
--checkand--non-interactivedo not prompt or hang, and that flag-form delegation matches the positional subcommand.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/continuous-test-suite-bugfixes.ts | Adds subprocess E2E-style regression tests against node dist/cli/index.js for setup --provider ... --check/--non-interactive. |
| src/lib/types/cli.ts | Adds check and nonInteractive fields to SetupArgs so the handler can read forwarded flags. |
| src/cli/commands/setup.ts | Plumbs check/nonInteractive through the delegation layer and mirrors "non-interactive" for compatibility with existing provider setup handlers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review Summary for PR #1343Decision: ✅ APPROVED This is a clean, minimal bug fix that addresses the issue where Changes ReviewedFile 1:
File 2:
File 3:
Impact Analysis
Quality Assessment✅ Type Safety: Properly typed, no Focus Areas Checked
Scope: This PR fixes a critical bug in CLI setup where important flags were being silently dropped, causing interactive prompts to hang or crash in CI environments. The fix is minimal, well-tested, and follows established patterns in the codebase. |
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary for PR #1343
Decision: ✅ APPROVED
This is a clean, minimal bug fix that addresses the issue where --check and --non-interactive flags were being dropped when calling provider-specific setup commands.
Changes Reviewed
File 1: src/cli/commands/setup.ts
- Added forwarding of
argv.checkandargv.nonInteractivetodelegateToProviderSetup() - Updated hardcoded
setupArgsobject to use passed flags instead of alwaysfalse - Clean implementation following existing patterns
File 2: src/lib/types/cli.ts
- Added two optional fields to
SetupArgstype:check?: boolean;andnonInteractive?: boolean; - No breaking change (only adding new optional properties)
File 3: test/continuous-test-suite-bugfixes.ts
- Added 3 comprehensive test cases verifying the fix
- Tests cover both flag form (
--provider openai --check) and subcommand form - Includes proper timeout handling and edge case testing
Impact Analysis
- Blast Radius: 139 files affected (all provider setup handlers receive these flags via
delegateToProviderSetup) - Affected Flows: CLI parser → setup command → provider delegation chain
- Breaking Changes: None (only adding optional parameters with defaults)
- Security Concerns: None
Quality Assessment
✅ Type Safety: Properly typed, no any usage
✅ No Breaking Changes: Only additive changes
✅ Testing: Comprehensive test coverage added
✅ Code Quality: Follows existing patterns, clean implementation
✅ Documentation: Tests serve as documentation for expected behavior
Focus Areas Checked
- ✅ Security: No issues
- ✅ Architecture: No CLAUDE.md violations
- ✅ Type Safety: Proper typing throughout
- ✅ Testing: New tests verify the fix
- ✅ Performance: No performance impact (pure flag forwarding)
Scope: This PR fixes a critical bug in CLI setup where important flags were being silently dropped, causing interactive prompts to hang or crash in CI environments. The fix is minimal, well-tested, and follows established patterns in the codebase.
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary for PR #1343
Decision: ✅ APPROVED
This is a clean, minimal bug fix that addresses the issue where --check and --non-interactive flags were being dropped when calling provider-specific setup commands.
Changes Reviewed
File 1: src/cli/commands/setup.ts
- Added forwarding of
argv.checkandargv.nonInteractivetodelegateToProviderSetup() - Updated hardcoded
setupArgsobject to use passed flags instead of alwaysfalse - Clean implementation following existing patterns
File 2: src/lib/types/cli.ts
- Added two optional fields to
SetupArgstype:check?: boolean;andnonInteractive?: boolean; - No breaking change (only adding new optional properties)
File 3: test/continuous-test-suite-bugfixes.ts
- Added 3 comprehensive test cases verifying the fix
- Tests cover both flag form (
--provider openai --check) and subcommand form - Includes proper timeout handling and edge case testing
Impact Analysis
- Blast Radius: 139 files affected (all provider setup handlers receive these flags via
delegateToProviderSetup) - Affected Flows: CLI parser → setup command → provider delegation chain
- Breaking Changes: None (only adding optional parameters with defaults)
- Security Concerns: None
Quality Assessment
✅ Type Safety: Properly typed, no any usage
✅ No Breaking Changes: Only additive changes
✅ Testing: Comprehensive test coverage added
✅ Code Quality: Follows existing patterns, clean implementation
✅ Documentation: Tests serve as documentation for expected behavior
Focus Areas Checked
- ✅ Security: No issues
- ✅ Architecture: No CLAUDE.md violations
- ✅ Type Safety: Proper typing throughout
- ✅ Testing: New tests verify the fix
- ✅ Performance: No performance impact (pure flag forwarding)
Scope: This PR fixes a critical bug in CLI setup where important flags were being silently dropped, causing interactive prompts to hang or crash in CI environments. The fix is minimal, well-tested, and follows established patterns in the codebase.
…er delegation neurolink setup --provider <p> --check silently ignored --check and --non-interactive: delegateToProviderSetup() hardcoded both to false when building the argv it hands to the per-provider setup handlers, so a credential that was already configured fell through to an interactive "reconfigure?" prompt instead of the check-only report. With no TTY attached that prompt aborted and exited non-zero. SetupArgs also lacked check/nonInteractive fields, so handleSetup had no typed way to read them off argv in the first place. delegateToProviderSetup now accepts the caller's flags (defaulting to the previous hardcoded values so the interactive wizard path is unchanged), and handleSetup forwards argv.check/argv.nonInteractive. The kebab-case "non-interactive" duplicate is kept in sync since two provider handlers read that key instead of the camelCase one.
8c6345d to
93d9038
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🔍 Code Review Summary for PR #1343 This PR fixes issue #1265 where Files Changed (3):
Review Findings:✅ No issues found - All changes are correct and well-implemented Positive observations:
Impact Assessment:
Verdict: APPROVED ✅The bug fix is straightforward, well-tested, and follows NeuroLink standards. Ready to merge. |
|
🎉 This PR is included in version 11.1.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
neurolink setup --provider openai --checkignores--check. It falls into the interactive "reconfigure?" prompt, and with no TTY — a script, a CI job — it crashes withUser force closed the promptand exits 1.--non-interactiveis dropped the same way. Confirmed against the built CLI, not by reading.The positional form
neurolink setup openai --checkworks correctly, because it dispatches straight to the provider handler and never passes throughhandleSetup. That was the reference for correct behavior.Cause
Not a missing flag declaration — yargs accepts both flags today. The break is in the handler layer:
SetupArgshad nocheck/nonInteractivefields, sohandleSetuphad no typed way to read what the user passed.delegateToProviderSetuphardcodedcheck: falseandnonInteractive: falseregardless of argv, then handed that to the per-provider handlers — which do honor the flags when they receive them.Pre-existing since 2025-09-09.
Fix
Adds the two fields to
SetupArgs, forwards them fromhandleSetup, and givesdelegateToProviderSetupa second parameter that defaults to the previous hardcoded values, so the interactive-wizard call site is byte-identical. The kebab-case"non-interactive"duplicate is kept in sync, because two handlers read that key rather than the camelCase one.Tests
Three tests added to the existing CLI-subprocess suite, driving
node dist/cli/index.js:--checktakes the check-only path without prompting or hanging; the flag form matches the dedicated positional subcommand's--check; and--non-interactiveworks on its own. They use a provider whose check path reads only env vars, a fixed fake key for determinism, and closed stdin so a regression reproduces the original hang rather than waiting on a terminal.Verified failing before the fix: with the source change stashed and rebuilt, all three fail for real (275 passed / 3 failed, non-zero exit). After: 278/278.
Noted, not changed
CLICommandFactory.createSetupCommand()incommandFactory.tsis dead code —parser.tsregisters thesetupCommandFactoryversion and never calls it. Deleting it is a separate cleanup.setup --listexits 1 andsetup --statushangs. Both are broken today, independent of this change, and confirmed unaffected by it.CI on this branch will fail at
Install ffmpeguntil #1342 lands; that failure is unrelated to this change.Summary by CodeRabbit
Bug Fixes
--checkcorrectly performs a non-interactive validation without prompting or hanging.--non-interactiveto reuse existing provider configuration without prompting.Tests