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 8 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().
There was a problem hiding this comment.
Fixed in commit 3f22099. Removed the duplicate 🔒 LiteLLM integration required banner from litellm.setup-steps.ts (lines 18–21). The spec-mandated 📌 banner in setup.ts remains the single enforcement banner — the redundant second one that used to fire immediately after getCredentials() was called is gone.
| 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.
There was a problem hiding this comment.
Fixed in commit 3f22099. enforced.codeMieUrl is now interpolated directly into the API-key prompt message and validator hint in litellm.setup-steps.ts — the prompt reads API Key for integration "{alias}" (required) — retrieve it from {codeMieUrl}:. The threaded portal URL now reaches the user at the display site.
| } 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.
There was a problem hiding this comment.
Both fixed in commit 3f22099.
- Extracted the abort check into
isPromptAbortError(error: unknown): booleanwhich useserror instanceof Error && (error.name === 'ExitPromptError' || error.name === 'AbortPromptError')— no moreanycast. - Wrapped
await detectLiteLLMEnforcement(existingCodeMieUrl)at the wizard call site (previously line 247) in atry/catchthat callsisPromptAbortError. On abort it printschalk.yellow('\nSetup cancelled.\n')and returns — matching the existingaction === 'cancel'behaviour — so Ctrl+C during SSO no longer surfaces as a raw stack trace through the Commander action handler.
Also added a new wizard-level test (handles ExitPromptError from the enforcement gate cleanly) that drives createSetupCommand().parseAsync() with a rejecting promptForCodeMieUrl and asserts the wizard resolves without calling getCredentials, catching any regression to the previous unguarded behaviour.
| 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.
There was a problem hiding this comment.
Fixed in commit 3f22099. runSetupWizard now loads the selected profile when isUpdate && profileName and threads its stored codeMieUrl as existingCodeMieUrl into detectLiteLLMEnforcement(existingCodeMieUrl):
let existingCodeMieUrl: string | undefined;
if (isUpdate && profileName) {
const existingProfile = await ConfigLoader.getProfile(profileName);
existingCodeMieUrl = existingProfile?.codeMieUrl;
}
enforcementResult = await detectLiteLLMEnforcement(existingCodeMieUrl);codemie setup --update no longer re-prompts users for a portal URL they already configured. Added a detectLiteLLMEnforcement unit test asserting the argument is forwarded verbatim to promptForCodeMieUrl.
| 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.
There was a problem hiding this comment.
Fixed in commit 3f22099. Removed the export { runSetupWizard as runSetupWizardForTest } line — runSetupWizard is once again a private module-scope function. The tests now drive through the public module boundary via Commander:
const command = setupModule.createSetupCommand();
await command.parseAsync([], { from: 'user' });All wizard-level test cases (auto-selects litellm..., uses normal provider prompt..., handles ExitPromptError...) were converted to this pattern. No public API leak, and the coverage is actually stronger since the tests now exercise the Commander action's argument handling too.
| 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.
There was a problem hiding this comment.
All three gaps fixed in commit 3f22099.
-
codeMieUrl assertion — the wizard-level test now asserts
codeMieUrl: 'https://codemie.example.com'insideexpect.objectContaining({ enforcedIntegration: expect.objectContaining({ alias: 'forced-int', codeMieUrl: 'https://codemie.example.com' }) }). ThedetectLiteLLMEnforcement"returns enforced:true" test also assertsresult.codeMieUrlexplicitly. A regression toauthResult.apiUrl(the CR-004 shape) would now fail both tests. -
ExitPromptError re-throw — two new tests: one where
promptForCodeMieUrlrejects with anExitPromptError(asserts the promise rejects andauthenticateWithCodeMieis never called), and one whereselectCodeMieProjectrejects with anExitPromptError(assertsfetchCodeMieIntegrationsis never called). Both use a localclass ExitPromptError extends Errorwiththis.name = 'ExitPromptError'mirroring inquirer's shape. -
Non-LiteLLM credential_type — new test
returns enforced:false when the only project integration is NOT credential_type=LiteLLM, using{ credential_type: 'GitHub', project_name: 'my-project' }. Removing thecredential_type === 'LiteLLM'filter from the predicate would make this test fail.
| 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.
There was a problem hiding this comment.
Fixed in commit 3f22099. Replaced enforced ? (key ?? '') : (key || 'not-required') with enforced ? key : (key || 'not-required'). With the pre-return guard if (enforced && !key) throw ... above, TypeScript narrows key to a non-empty string in the enforced branch, so the ?? '' fallback was unreachable and misleading.
Addresses reviewer feedback on commit 72d3d82. setup.ts: - Replace `(error as any)?.name` with `error instanceof Error` narrowing via isPromptAbortError() helper — complies with the no-any policy in AGENTS.md. - Wrap detectLiteLLMEnforcement() at the wizard call site in a try/catch that handles ExitPromptError cleanly (`Setup cancelled.` + return) instead of surfacing a raw stack trace through the Commander action. - Load the selected profile on update flows and thread its stored codeMieUrl to detectLiteLLMEnforcement() as existingCodeMieUrl, so `codemie setup --update` does not re-prompt for a portal URL the user already configured. - Drop the runSetupWizardForTest alias — tests now drive through the createSetupCommand() module boundary via Commander parseAsync. litellm.setup-steps.ts: - Remove the duplicate LiteLLM banner from getCredentials(); the spec-mandated banner already prints once from setup.ts before this step runs. - Interpolate enforced.codeMieUrl directly into the API-key prompt message and validator hint so the threaded portal URL actually reaches the user. - Simplify the return: with the pre-return guard in place, `key ?? ''` is dead code; use `key` directly in the enforced branch. setup.enforcement.test.ts: - Rewrite the second describe block to invoke createSetupCommand and parseAsync instead of the removed runSetupWizardForTest export. - Strengthen the SetupContext assertion to check codeMieUrl explicitly so a regression to authResult.apiUrl fails the test. - Add three new detectLiteLLMEnforcement tests: threads existingCodeMieUrl through to promptForCodeMieUrl, treats non-LiteLLM credential_type as not enforced, and re-throws ExitPromptError from both promptForCodeMieUrl and selectCodeMieProject. - Add a wizard-level test proving the setup wizard swallows ExitPromptError cleanly (no getCredentials call, no thrown stack trace). EPMCDME-11733
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