fix(cli): enforce LiteLLM integration in setup when project has one configured - #429
fix(cli): enforce LiteLLM integration in setup when project has one configured#429SleepySML wants to merge 7 commits into
Conversation
…nforcement threading
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ePluginSetup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Filter integrations by credential_type === 'LiteLLM' (CR-001) - Warn when multiple LiteLLM integrations match, use first (CR-002) - Skip SSO enforcement gate when isUpdate is true (CR-003) - Guard answers.apiKey with optional chain in enforcement mode (CR-004) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Includes spec, plan, technical-analysis, code-review reports, decisions/events audit logs, and .state.json (phase: maintenance). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
yanaSelin
left a comment
There was a problem hiding this comment.
Code review via SDLC Factory — 2 critical findings, 4 major findings. Full machine verdict: docs/superpowers/reviews/2026-07-28-pr-429/code-review-final.json
| const allProviderChoices = getAllProviderChoices(registeredProviders); | ||
| // Step 1: Check for mandatory LiteLLM integration before provider selection | ||
| // Skip SSO gate on update flows — enforcement only applies to fresh configurations. | ||
| const enforcementResult = isUpdate ? { enforced: false as const } : await detectLiteLLMEnforcement(); |
There was a problem hiding this comment.
[CR-001] Critical — --force bypass of enforcement gate
The spec explicitly states: "--force flag still works; enforcement is re-checked live every time." This ternary skips detectLiteLLMEnforcement() on all update/--force flows, so users can configure any provider on a project that requires LiteLLM by simply running codemie setup --force.
| const enforcementResult = isUpdate ? { enforced: false as const } : await detectLiteLLMEnforcement(); | |
| const enforcementResult = await detectLiteLLMEnforcement(); |
If a genuine "skip on update-only (no --force)" behaviour is needed in the future, that should be a separate explicit flag with a documented spec deviation — not a side effect of isUpdate.
There was a problem hiding this comment.
Fixed in commit 72d3d82. Removed the isUpdate ternary guard entirely — detectLiteLLMEnforcement() now runs unconditionally on every setup flow, including updates and --force. The old isUpdate ? { enforced: false as const } : await detectLiteLLMEnforcement() has been replaced with a direct await detectLiteLLMEnforcement() call.
| if (enforcementResult.enforced) { | ||
| const litellmSteps = ProviderRegistry.getSetupSteps('litellm'); | ||
| if (!litellmSteps) { | ||
| throw new Error('LiteLLM provider is required by your project integration but is not configured'); |
There was a problem hiding this comment.
[CR-002] Major — error message missing actionable recovery guidance
The spec requires this exact throw message:
"LiteLLM integration is required for this project but the LiteLLM provider is not available. Please reinstall codemie-cli."
The current message omits "Please reinstall codemie-cli." — users hitting this hard error (all prompts already shown) have no guidance on how to recover.
| throw new Error('LiteLLM provider is required by your project integration but is not configured'); | |
| throw new Error('LiteLLM integration is required for this project but the LiteLLM provider is not available. Please reinstall codemie-cli.'); |
There was a problem hiding this comment.
Fixed in commit 72d3d82. The throw message now matches the spec exactly: 'LiteLLM integration is required for this project but the LiteLLM provider is not available. Please reinstall codemie-cli.' — the missing "Please reinstall codemie-cli." suffix has been added.
| project: enforcementResult.project, | ||
| authResult: enforcementResult.authResult | ||
| }; | ||
| console.log(chalk.cyan(`\n🔒 LiteLLM integration "${enforcementResult.integration.alias}" is required for project "${enforcementResult.project}". Proceeding with LiteLLM setup.\n`)); |
There was a problem hiding this comment.
[CR-003] Critical — enforcement banner text doesn't match spec
The spec mandates the exact string:
📌 This project uses a mandatory LiteLLM integration: "{alias}"
Provider has been set to LiteLLM automatically.
Current banner has the wrong emoji (🔒 vs 📌), a different message body, and adds a {project} interpolation not in the spec. The "Provider has been set to LiteLLM automatically." confirmation is also missing.
| console.log(chalk.cyan(`\n🔒 LiteLLM integration "${enforcementResult.integration.alias}" is required for project "${enforcementResult.project}". Proceeding with LiteLLM setup.\n`)); | |
| console.log(chalk.cyan(`\n📌 This project uses a mandatory LiteLLM integration: "${enforcementResult.integration.alias}"\n Provider has been set to LiteLLM automatically.\n`)); |
There was a problem hiding this comment.
Fixed in commit 72d3d82. The banner now uses the spec-mandated 📌 emoji and exact text: "📌 This project uses a mandatory LiteLLM integration: \"${alias}\"\n Provider has been set to LiteLLM automatically." — the wrong 🔒 emoji is gone, {project} interpolation removed, and the "Provider has been set" confirmation line added.
There was a problem hiding this comment.
Partial fix. setup.ts is correct now, but litellm.setup-steps.ts introduces a second banner (line 19) that fires immediately after the first one. The user sees two consecutive banners for the same enforcement event. The redundancy is the issue — one of the two should be removed:
- Remove the banner from
getCredentials()entirely (sincerunSetupWizardalready printed one before calling it), or - Remove the
📌banner fromsetup.tsand keep only the one insidegetCredentials().
| enforcedIntegration: { | ||
| id: enforcementContext.integration.id, | ||
| alias: enforcementContext.integration.alias, | ||
| codeMieUrl: enforcementContext.authResult.apiUrl! |
There was a problem hiding this comment.
[CR-004] Major — API endpoint URL stored as portal URL in SetupContext
authResult.apiUrl is the REST API base URL (e.g. https://codemie.example.com/api) — not the user-facing portal. SetupContext.codeMieUrl is named and documented as the portal URL where users go to retrieve API keys (shown in the message: "Get your API key from your CodeMie portal").
When codeMieUrl is eventually used to construct a portal link, users will land on the wrong URL.
Fix: thread the portal URL through LiteLLMEnforcementContext:
// In detectLiteLLMEnforcement, capture the portal URL:
// const codeMieUrl = await promptForCodeMieUrl(...) ← this is the portal URL
// Return it alongside authResult: { ..., codeMieUrl }
// In handlePluginSetup:
codeMieUrl: enforcementContext.codeMieUrl // ← portal URL, not authResult.apiUrlThere was a problem hiding this comment.
Fixed in commit 72d3d82. detectLiteLLMEnforcement() now captures the portal URL from promptForCodeMieUrl() and returns it as codeMieUrl in LiteLLMEnforcementContext. This user-facing URL is then threaded through enforcedIntegration.codeMieUrl in SetupContext and used directly in handlePluginSetup — replacing the incorrect authResult.apiUrl (the REST API base) that was previously passed.
There was a problem hiding this comment.
Partial fix. codeMieUrl is correctly threaded all the way to SetupContext.enforcedIntegration.codeMieUrl, but it is never interpolated into any user-facing string.
litellm.setup-steps.ts lines 19–20 print a generic banner and the validator (line 41) says Retrieve it from your CodeMie portal. — both ignore enforced.codeMieUrl. The value is plumbed correctly but discarded at the display site. The original intent — showing which portal URL to open — is not delivered. Include enforced.codeMieUrl in the banner or validator message.
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| logger.warn(`Could not check for mandatory integrations: ${errorMessage}`); | ||
| console.log(chalk.yellow(`\n⚠️ Could not check for mandatory integrations (${errorMessage}). Continuing with normal provider setup.\n`)); |
There was a problem hiding this comment.
[CR-005] Major — Ctrl+C during SSO prompt silently bypasses enforcement
When the user presses Ctrl+C during any inquirer prompt inside detectLiteLLMEnforcement, inquirer throws an ExitPromptError. This catch block swallows it and returns { enforced: false } — treating an intentional abort as "couldn't check integrations, continuing normally." The user can now pick any provider on a project that requires LiteLLM.
| console.log(chalk.yellow(`\n⚠️ Could not check for mandatory integrations (${errorMessage}). Continuing with normal provider setup.\n`)); | |
| } catch (error) { | |
| if ((error as any)?.name === 'ExitPromptError' || (error as any)?.name === 'AbortPromptError') { | |
| throw error; | |
| } | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| logger.warn(`Could not check for mandatory integrations: ${errorMessage}`); | |
| console.log(chalk.yellow(`\n⚠️ Could not check for mandatory integrations (${errorMessage}). Continuing with normal provider setup.\n`)); | |
| return { enforced: false }; | |
| } |
There was a problem hiding this comment.
Fixed in commit 72d3d82. The catch block now checks error?.name === 'ExitPromptError' || error?.name === 'AbortPromptError' and re-throws immediately before the generic handler runs. Ctrl+C during the SSO prompt now propagates correctly and aborts the setup flow instead of silently returning { enforced: false }.
| return { | ||
| baseUrl: answers.baseUrl.trim(), | ||
| apiKey: answers.apiKey?.trim() || 'not-required' | ||
| apiKey: enforced ? (answers.apiKey?.trim() ?? '') : (answers.apiKey?.trim() || 'not-required') |
There was a problem hiding this comment.
[CR-006] Major — empty API key persists in enforcement mode when validator is bypassed
In interactive TTY use the validate callback prevents empty input. In non-TTY / programmatic invocations (CI, tests calling getCredentials directly) inquirer doesn't run validators, so answers.apiKey can be undefined — and undefined?.trim() ?? '' silently returns '', which gets stored as the API key for a mandatory integration.
| apiKey: enforced ? (answers.apiKey?.trim() ?? '') : (answers.apiKey?.trim() || 'not-required') | |
| apiKey: (() => { | |
| const key = answers.apiKey?.trim(); | |
| if (enforced && !key) throw new Error('API Key is required for this integration.'); | |
| return enforced ? (key ?? '') : (key || 'not-required'); | |
| })() |
Alternatively, a simple post-prompt guard before the return block is enough.
There was a problem hiding this comment.
Fixed in commit 72d3d82. Added a post-prompt guard after inquirer.prompt() returns:
const key = answers.apiKey?.trim();
if (enforced && !key) throw new Error('API Key is required for this integration.');
This catches the case where validate is bypassed in non-TTY/programmatic contexts (CI, tests). The return value uses enforced ? (key ?? '') : (key || 'not-required') to make the enforcement branch explicit — an empty key in enforcement mode now throws rather than silently falling through to 'not-required'.
- CR-001: Remove isUpdate guard; enforcement runs on all flows per spec - CR-002: Add "Please reinstall codemie-cli." to provider-unavailable error - CR-003: Fix banner to use 📌 emoji and spec-exact message text - CR-004: Thread portal URL through LiteLLMEnforcementContext.codeMieUrl instead of using authResult.apiUrl (REST API URL, not portal) - CR-005: Re-throw ExitPromptError/AbortPromptError in catch block so Ctrl+C cannot silently bypass enforcement - CR-006: Add post-prompt guard in litellm.setup-steps.ts to reject empty API key in enforcement mode regardless of TTY context Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| } | ||
| return { enforced: true, integration: projectIntegrations[0], project, authResult, codeMieUrl }; | ||
| } catch (error) { | ||
| if ((error as any)?.name === 'ExitPromptError' || (error as any)?.name === 'AbortPromptError') { |
There was a problem hiding this comment.
Two issues here:
1. no any violation — AGENTS.md forbids any. The correct narrowing for unknown in a catch block is:
if (error instanceof Error && (error.name === "ExitPromptError" || error.name === "AbortPromptError")) {2. Unguarded propagation — the re-thrown ExitPromptError surfaces at the await detectLiteLLMEnforcement() call in runSetupWizard (line 247), which has no surrounding try/catch. If Commander does not intercept it by name, Ctrl+C produces a raw stack trace instead of a clean cancellation.
| const registeredProviders = ProviderRegistry.getAllProviders(); | ||
| const allProviderChoices = getAllProviderChoices(registeredProviders); | ||
| // Step 1: Check for mandatory LiteLLM integration before provider selection | ||
| const enforcementResult = await detectLiteLLMEnforcement(); |
There was a problem hiding this comment.
detectLiteLLMEnforcement() is always called with no argument, so existingCodeMieUrl inside the function is always undefined. On codemie setup --update, the user is prompted to re-enter the CodeMie portal URL even when one is already stored in the active profile. Pass the existing configured URL here so update flows skip the URL prompt.
| return result; | ||
| } | ||
|
|
||
| export { runSetupWizard as runSetupWizardForTest }; |
There was a problem hiding this comment.
Exporting runSetupWizard as runSetupWizardForTest leaks a private function as a public module API solely to support tests. Any consumer of setup.js can now call runSetupWizardForTest(), bypassing all CLI argument parsing.
Per the project architecture (CLI → Registry → Plugin), internal functions should stay unexported. Tests should intercept at module boundaries using vi.mock() rather than requiring a dedicated test export.
| expect(mockGetCredentials).toHaveBeenCalledWith( | ||
| false, | ||
| expect.objectContaining({ | ||
| enforcedIntegration: expect.objectContaining({ alias: 'forced-int' }) |
There was a problem hiding this comment.
Three gaps in test coverage:
1. This assertion — expect.objectContaining({ alias: "forced-int" }) does not assert codeMieUrl. If the portal URL is accidentally replaced with authResult.apiUrl again (the original CR-004 bug), this test still passes.
2. Missing: ExitPromptError re-throw — none of the five detectLiteLLMEnforcement tests simulate promptForCodeMieUrl or selectCodeMieProject throwing an ExitPromptError. The only code path added by the CR-005 fix is untested.
3. Missing: non-LiteLLM credential_type — every integration mock uses credential_type: "LiteLLM". There is no test asserting that credential_type: "GitHub" (or any other type) returns enforced: false. Removing the type filter from the predicate would be invisible to the test suite.
| return { | ||
| baseUrl: answers.baseUrl.trim(), | ||
| apiKey: answers.apiKey?.trim() || 'not-required' | ||
| apiKey: enforced ? (key ?? '') : (key || 'not-required') |
There was a problem hiding this comment.
key ?? "" is dead code. The guard two lines above (if (enforced && !key) throw ...) ensures key is a non-empty string whenever enforced is truthy before this line runs. The ?? "" fallback is unreachable and misleads readers about the invariant. Replace with just key.
Summary
Implements EPMCDME-11733: when a CodeMie project already has a LiteLLM integration,
codemie setupnow detects it before the provider-selection prompt and forces the user through the LiteLLM path — they cannot complete setup without providing the required API key.Changes
src/providers/core/types.ts— addsSetupContextinterface (enforcedIntegration?: { id, alias, codeMieUrl }) and threads it intoProviderSetupSteps.getCredentialssrc/providers/plugins/litellm/litellm.setup-steps.ts— enforces non-empty API key whencontext.enforcedIntegrationis set; suppresses'not-required'fallbacksrc/cli/commands/setup.ts— addsdetectLiteLLMEnforcement()gate (SSO auth → project select → fetch integrations → filter byproject_nameandcredential_type === 'LiteLLM') that runs before provider selection; graceful fallback on any SSO error; update flows skip the gate (isUpdateguard)src/cli/commands/__tests__/setup.enforcement.test.ts— new; 7 tests covering the enforcement gate and its wiring into the wizardsrc/providers/plugins/litellm/__tests__/litellm.setup-steps.test.ts— new; 5 tests covering enforcement and normal modesTesting
npm run test:unit— 2329/2330 pass; 1 pre-existing unrelated failure)npm run lint— zero warnings)npm run typecheck)npm run build)Checklist
npm run ci— pre-existingself-update.test.tsfailure unrelated to this change)mainCloses EPMCDME-11733