Skip to content

fix(cli): enforce LiteLLM integration in setup when project has one configured - #429

Open
SleepySML wants to merge 7 commits into
codemie-ai:mainfrom
SleepySML:EPMCDME-11733
Open

fix(cli): enforce LiteLLM integration in setup when project has one configured#429
SleepySML wants to merge 7 commits into
codemie-ai:mainfrom
SleepySML:EPMCDME-11733

Conversation

@SleepySML

Copy link
Copy Markdown
Contributor

Summary

Implements EPMCDME-11733: when a CodeMie project already has a LiteLLM integration, codemie setup now 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 — adds SetupContext interface (enforcedIntegration?: { id, alias, codeMieUrl }) and threads it into ProviderSetupSteps.getCredentials
  • src/providers/plugins/litellm/litellm.setup-steps.ts — enforces non-empty API key when context.enforcedIntegration is set; suppresses 'not-required' fallback
  • src/cli/commands/setup.ts — adds detectLiteLLMEnforcement() gate (SSO auth → project select → fetch integrations → filter by project_name and credential_type === 'LiteLLM') that runs before provider selection; graceful fallback on any SSO error; update flows skip the gate (isUpdate guard)
  • src/cli/commands/__tests__/setup.enforcement.test.ts — new; 7 tests covering the enforcement gate and its wiring into the wizard
  • src/providers/plugins/litellm/__tests__/litellm.setup-steps.test.ts — new; 5 tests covering enforcement and normal modes

Testing

  • Unit tests added (npm run test:unit — 2329/2330 pass; 1 pre-existing unrelated failure)
  • Lint clean (npm run lint — zero warnings)
  • TypeScript clean (npm run typecheck)
  • Build passes (npm run build)
  • Manual staging verification (enforcement gate against a real CodeMie project with LiteLLM integration)

Checklist

  • Code follows project standards
  • CI gates pass (npm run ci — pre-existing self-update.test.ts failure unrelated to this change)
  • No merge conflicts with main

Closes EPMCDME-11733

EvgeniiKurdakov and others added 6 commits July 20, 2026 14:46
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 yanaSelin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread src/cli/commands/setup.ts Outdated
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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.

Suggested change
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread src/cli/commands/setup.ts Outdated
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');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.

Suggested change
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.');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread src/cli/commands/setup.ts Outdated
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`));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.

Suggested change
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`));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 (since runSetupWizard already printed one before calling it), or
  • Remove the 📌 banner from setup.ts and keep only the one inside getCredentials().

Comment thread src/cli/commands/setup.ts Outdated
enforcedIntegration: {
id: enforcementContext.integration.id,
alias: enforcementContext.integration.alias,
codeMieUrl: enforcementContext.authResult.apiUrl!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.apiUrl

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/cli/commands/setup.ts
} 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`));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.

Suggested change
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 };
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.

Suggested change
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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>
Comment thread src/cli/commands/setup.ts
}
return { enforced: true, integration: projectIntegrations[0], project, authResult, codeMieUrl };
} catch (error) {
if ((error as any)?.name === 'ExitPromptError' || (error as any)?.name === 'AbortPromptError') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/cli/commands/setup.ts
const registeredProviders = ProviderRegistry.getAllProviders();
const allProviderChoices = getAllProviderChoices(registeredProviders);
// Step 1: Check for mandatory LiteLLM integration before provider selection
const enforcementResult = await detectLiteLLMEnforcement();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/cli/commands/setup.ts
return result;
}

export { runSetupWizard as runSetupWizardForTest };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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' })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Three gaps in test coverage:

1. This assertionexpect.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')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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.

3 participants