diff --git a/apps/fabro-web/app/install-app.test.tsx b/apps/fabro-web/app/install-app.test.tsx index 2ca421809..07025a3fa 100644 --- a/apps/fabro-web/app/install-app.test.tsx +++ b/apps/fabro-web/app/install-app.test.tsx @@ -904,4 +904,382 @@ describe("InstallApp", () => { console.error = originalConsoleError; } }); + + test("skips LLM setup with an empty providers list and advances to GitHub", async () => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + const originalConsoleError = console.error; + console.error = ((...args: unknown[]) => { + if ( + typeof args[0] === "string" && + args[0].startsWith("react-test-renderer is deprecated") + ) { + return; + } + originalConsoleError(...args); + }) as typeof console.error; + try { + const fetchCalls: Array<{ input: RequestInfo | URL; init?: RequestInit }> = []; + const fetchMock = mock((input: RequestInfo | URL, init?: RequestInit) => { + fetchCalls.push({ input, init }); + if (String(input) === "/install/session" && fetchCalls.length === 1) { + return Promise.resolve( + new Response( + JSON.stringify({ + completed_steps: ["server", "object_store", "sandbox"], + llm: null, + server: { canonical_url: "https://fabro.example.com" }, + object_store: { provider: "local" }, + sandbox: { provider: "docker" }, + github: null, + prefill: INSTALL_PREFILL, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), + ); + } + if (String(input) === "/install/llm") { + return Promise.resolve(new Response(null, { status: 204 })); + } + if (String(input) === "/install/session") { + return Promise.resolve( + new Response( + JSON.stringify({ + completed_steps: ["server", "object_store", "sandbox", "llm"], + llm: { providers: [] }, + server: { canonical_url: "https://fabro.example.com" }, + object_store: { provider: "local" }, + sandbox: { provider: "docker" }, + github: null, + prefill: INSTALL_PREFILL, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), + ); + } + throw new Error(`unexpected fetch: ${String(input)}`); + }); + useInstallFetchMock(fetchMock as typeof fetch); + + const testWindow = createTestWindow("https://fabro.example.com/install/llm"); + testWindow.sessionStorage.setItem("fabro-install-token", "test-install-token"); + (globalThis as { window?: unknown }).window = testWindow; + + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + renderer = TestRenderer.create( + + + } /> + + , + ); + }); + + await waitFor(() => { + expect(renderTreeText(renderer!.toJSON())).toContain("Add your LLM credentials"); + }); + + const skipButton = renderer!.root.findAll( + (node) => node.type === "button" && node.children.includes("Skip LLM setup"), + )[0]; + expect(skipButton).toBeDefined(); + await act(async () => { + skipButton!.props.onClick(); + }); + + await waitFor(() => { + expect(renderTreeText(renderer!.toJSON())).toContain("Connect GitHub"); + }); + + const calls = fetchCalls.map((call) => String(call.input)); + const putIdx = calls.indexOf("/install/llm"); + expect(putIdx).toBeGreaterThanOrEqual(0); + expect(fetchCalls[putIdx]?.init?.body).toBe(JSON.stringify({ providers: [] })); + + await act(async () => { + renderer?.unmount(); + }); + } finally { + console.error = originalConsoleError; + } + }); + + test("blocks Continue on the LLM step when no API keys are entered", async () => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + const originalConsoleError = console.error; + console.error = ((...args: unknown[]) => { + if ( + typeof args[0] === "string" && + args[0].startsWith("react-test-renderer is deprecated") + ) { + return; + } + originalConsoleError(...args); + }) as typeof console.error; + try { + const fetchMock = mock((input: RequestInfo | URL) => { + if (String(input) === "/install/session") { + return Promise.resolve( + new Response( + JSON.stringify({ + completed_steps: ["server", "object_store", "sandbox"], + llm: null, + server: { canonical_url: "https://fabro.example.com" }, + object_store: { provider: "local" }, + sandbox: { provider: "docker" }, + github: null, + prefill: INSTALL_PREFILL, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), + ); + } + throw new Error(`unexpected fetch: ${String(input)}`); + }); + useInstallFetchMock(fetchMock as typeof fetch); + + const testWindow = createTestWindow("https://fabro.example.com/install/llm"); + testWindow.sessionStorage.setItem("fabro-install-token", "test-install-token"); + (globalThis as { window?: unknown }).window = testWindow; + + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + renderer = TestRenderer.create( + + + } /> + + , + ); + }); + + await waitFor(() => { + expect(renderTreeText(renderer!.toJSON())).toContain("Add your LLM credentials"); + }); + + const form = renderer!.root.findByType("form"); + await act(async () => { + form.props.onSubmit({ preventDefault() {} }); + }); + + await waitFor(() => { + expect(renderTreeText(renderer!.toJSON())).toContain( + "Add at least one provider API key before continuing.", + ); + }); + + await act(async () => { + renderer?.unmount(); + }); + } finally { + console.error = originalConsoleError; + } + }); + + test("shows a skipped LLM step as Skipped on the review step", async () => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + const originalConsoleError = console.error; + console.error = ((...args: unknown[]) => { + if ( + typeof args[0] === "string" && + args[0].startsWith("react-test-renderer is deprecated") + ) { + return; + } + originalConsoleError(...args); + }) as typeof console.error; + try { + const fetchMock = mock((input: RequestInfo | URL) => { + expect(String(input)).toBe("/install/session"); + return Promise.resolve( + new Response( + JSON.stringify({ + completed_steps: ["server", "object_store", "sandbox", "llm", "github"], + llm: { providers: [] }, + server: { canonical_url: "https://fabro.example.com" }, + object_store: { provider: "local" }, + sandbox: { provider: "docker" }, + github: { strategy: "token", username: "octocat" }, + prefill: INSTALL_PREFILL, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), + ); + }); + useInstallFetchMock(fetchMock as typeof fetch); + + const testWindow = createTestWindow("https://fabro.example.com/install/review"); + testWindow.sessionStorage.setItem("fabro-install-token", "test-install-token"); + (globalThis as { window?: unknown }).window = testWindow; + + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + renderer = TestRenderer.create( + + + } /> + + , + ); + }); + + await waitFor(() => { + const text = renderTreeText(renderer!.toJSON()); + expect(text).toContain("LLM providers"); + expect(text).toContain("Skipped"); + }); + + await act(async () => { + renderer?.unmount(); + }); + } finally { + console.error = originalConsoleError; + } + }); + + test("shows an incomplete LLM step as Not configured on the review step", async () => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + const originalConsoleError = console.error; + console.error = ((...args: unknown[]) => { + if ( + typeof args[0] === "string" && + args[0].startsWith("react-test-renderer is deprecated") + ) { + return; + } + originalConsoleError(...args); + }) as typeof console.error; + try { + const fetchMock = mock((input: RequestInfo | URL) => { + expect(String(input)).toBe("/install/session"); + return Promise.resolve( + new Response( + JSON.stringify({ + completed_steps: ["server", "object_store", "sandbox", "github"], + llm: null, + server: { canonical_url: "https://fabro.example.com" }, + object_store: { provider: "local" }, + sandbox: { provider: "docker" }, + github: { strategy: "token", username: "octocat" }, + prefill: INSTALL_PREFILL, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), + ); + }); + useInstallFetchMock(fetchMock as typeof fetch); + + const testWindow = createTestWindow("https://fabro.example.com/install/review"); + testWindow.sessionStorage.setItem("fabro-install-token", "test-install-token"); + (globalThis as { window?: unknown }).window = testWindow; + + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + renderer = TestRenderer.create( + + + } /> + + , + ); + }); + + await waitFor(() => { + const text = renderTreeText(renderer!.toJSON()); + expect(text).toContain("LLM providers"); + expect(text).toContain("Not configured"); + }); + expect(renderTreeText(renderer!.toJSON())).not.toContain("Skipped"); + + await act(async () => { + renderer?.unmount(); + }); + } finally { + console.error = originalConsoleError; + } + }); + + test("keeps the user on the LLM step when skipping fails", async () => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + const originalConsoleError = console.error; + console.error = ((...args: unknown[]) => { + if ( + typeof args[0] === "string" && + args[0].startsWith("react-test-renderer is deprecated") + ) { + return; + } + originalConsoleError(...args); + }) as typeof console.error; + try { + const fetchCalls: Array<{ input: RequestInfo | URL; init?: RequestInit }> = []; + const fetchMock = mock((input: RequestInfo | URL, init?: RequestInit) => { + fetchCalls.push({ input, init }); + if (String(input) === "/install/session") { + return Promise.resolve( + new Response( + JSON.stringify({ + completed_steps: ["server", "object_store", "sandbox"], + llm: null, + server: { canonical_url: "https://fabro.example.com" }, + object_store: { provider: "local" }, + sandbox: { provider: "docker" }, + github: null, + prefill: INSTALL_PREFILL, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ), + ); + } + if (String(input) === "/install/llm") { + return Promise.resolve(new Response(null, { status: 500 })); + } + throw new Error(`unexpected fetch: ${String(input)}`); + }); + useInstallFetchMock(fetchMock as typeof fetch); + + const testWindow = createTestWindow("https://fabro.example.com/install/llm"); + testWindow.sessionStorage.setItem("fabro-install-token", "test-install-token"); + (globalThis as { window?: unknown }).window = testWindow; + + let renderer: TestRenderer.ReactTestRenderer | null = null; + await act(async () => { + renderer = TestRenderer.create( + + + } /> + + , + ); + }); + + await waitFor(() => { + expect(renderTreeText(renderer!.toJSON())).toContain("Add your LLM credentials"); + }); + + const skipButton = renderer!.root.findAll( + (node) => node.type === "button" && node.children.includes("Skip LLM setup"), + )[0]; + await act(async () => { + skipButton!.props.onClick(); + }); + + // The failed PUT must not advance to GitHub or refresh the session. + await waitFor(() => { + expect(fetchCalls.map((call) => String(call.input))).toContain("/install/llm"); + }); + const text = renderTreeText(renderer!.toJSON()); + expect(text).toContain("Add your LLM credentials"); + expect(text).not.toContain("Connect GitHub"); + expect( + fetchCalls.filter((call) => String(call.input) === "/install/session"), + ).toHaveLength(1); + + await act(async () => { + renderer?.unmount(); + }); + } finally { + console.error = originalConsoleError; + } + }); }); diff --git a/apps/fabro-web/app/install-app.tsx b/apps/fabro-web/app/install-app.tsx index f5d7595f8..2130d82de 100644 --- a/apps/fabro-web/app/install-app.tsx +++ b/apps/fabro-web/app/install-app.tsx @@ -304,6 +304,11 @@ export default function InstallApp() { fallback: string; next?: string; }) => { + // Re-entrancy guard: the StepPanel form guards its own onSubmit, but the + // LLM step's "Skip LLM setup" button calls this directly, so a fast + // double-click could otherwise fire two requests before `submitting` + // re-renders the disabled state. + if (submitting) return; setSubmitting(true); setSaveError(null); try { @@ -359,10 +364,26 @@ export default function InstallApp() { ) : location.pathname === "/install/llm" ? ( { + void runStepSubmit({ + action: () => putInstallLlm(installToken, []), + fallback: "Failed to skip LLM setup.", + next: "/install/github", + }); + }} + > + Skip LLM setup + + } onSubmit={async () => { const providers = INSTALL_PROVIDERS.map(({ id }) => { const current = llmSelection[id] ?? { apiKey: "" }; @@ -1164,6 +1185,7 @@ function StepPanel({ submitLabel = "Continue", submittingLabel = "Saving...", backHref, + secondaryAction, onSubmit, }: { title: string; @@ -1174,6 +1196,7 @@ function StepPanel({ submitLabel?: string; submittingLabel?: string; backHref?: string; + secondaryAction?: ReactNode; onSubmit: () => Promise; }) { return ( @@ -1204,19 +1227,22 @@ function StepPanel({ ) : ( )} - +
+ {secondaryAction} + +
); @@ -1233,9 +1259,7 @@ function ReviewScreen({ submitting: boolean; onInstall: () => Promise; }) { - const providers = (session?.llm?.providers ?? []) - .map((provider) => describeProvider(provider.provider)) - .join(", "); + const llmSummary = describeLlmSummary(session?.llm); const serverUrl = session?.server?.canonical_url || session?.prefill.canonical_url || "Unknown"; return ( @@ -1265,7 +1289,7 @@ function ReviewScreen({ /> {renderObjectStoreSummaryRows(session?.object_store)} {renderSandboxSummaryRows(session?.sandbox)} - + {renderGithubSummaryRows(session?.github, serverUrl)} {error ? : null} @@ -1872,6 +1896,18 @@ function describeProvider(id: string): string { return match?.label ?? id; } +function describeLlmSummary(llm: InstallSessionResponse["llm"]): string { + // `null` means the LLM step has not been completed. A present summary with + // an empty providers list is an explicit skip. + if (!llm) { + return "Not configured"; + } + const providers = (llm.providers ?? []).map((provider) => + describeProvider(provider.provider), + ); + return providers.length > 0 ? providers.join(", ") : "Skipped"; +} + function renderGithubSummaryRows( github: InstallSessionResponse["github"], serverUrl: string, diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 01f17ac0e..52f2e5bbd 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -143,7 +143,10 @@ paths: operationId: putInstallLlm tags: [Install] summary: Save install LLM settings - description: Records the LLM providers and API keys chosen during the browser install. Requires the one-time install token. + description: >- + Records the LLM providers and API keys chosen during the browser + install. An empty `providers` list marks the LLM step as completed + and explicitly skipped. Requires the one-time install token. security: [] requestBody: required: true @@ -3978,14 +3981,18 @@ components: type: string InstallLlmProvidersInput: - description: LLM providers selected during browser install. + description: >- + LLM providers selected during browser install. An empty `providers` + list explicitly marks the LLM step as completed and skipped. type: object required: - providers properties: providers: type: array - minItems: 1 + description: >- + LLM providers to persist. An empty list records an explicit skip: + the LLM step is marked complete with zero credentials. items: $ref: "#/components/schemas/InstallLlmProviderInput" @@ -4003,7 +4010,10 @@ components: type: string InstallLlmSummary: - description: Redacted summary of persisted LLM install choices. + description: >- + Redacted summary of persisted LLM install choices. Present with an + empty `providers` list when the LLM step was explicitly skipped; + `null` on the install session means the step is still incomplete. type: object properties: providers: diff --git a/docs/superpowers/plans/2026-05-14-optional-llm-install.md b/docs/superpowers/plans/2026-05-14-optional-llm-install.md new file mode 100644 index 000000000..874ab99b5 --- /dev/null +++ b/docs/superpowers/plans/2026-05-14-optional-llm-install.md @@ -0,0 +1,65 @@ +# Optional LLM Setup in Installers Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make LLM setup explicitly skippable in both the web installer and `fabro install`, while preserving current behavior for users who do configure LLMs. + +**Architecture:** Represent an explicit web-installer skip as a completed LLM step with an empty provider list. Represent an explicit CLI skip with an interactive confirmation path and a hidden non-interactive `--skip-llm` flag. Runtime model resolution and doctor behavior remain unchanged. + +**Tech Stack:** Rust CLI/server (`fabro-cli`, `fabro-server`, `fabro-api`), OpenAPI-generated clients, React/TypeScript web installer, Bun tests, cargo-nextest. + +--- + +## Summary + +Make LLM setup optional without making omission accidental. Users can explicitly skip LLM setup and still complete installation. A skipped LLM step means install can complete with zero LLM credentials; later LLM-dependent workflows keep using existing provider-not-configured behavior. Per product decision, `fabro doctor` remains unchanged and may report no LLM providers configured. + +## Key Changes + +- **Web install API:** Treat `PUT /install/llm` with `{"providers":[]}` as "LLM step completed, skipped." + - Remove `minItems: 1` from `InstallLlmProvidersInput.providers` in `docs/public/api-reference/fabro-api.yaml`. + - Update schema descriptions so empty `providers` explicitly means skipped. + - Keep `/install/finish` requiring the LLM step to be completed, but allow the completed step to contain zero providers. +- **Web UI:** Add an explicit "Skip LLM setup" secondary action on the LLM step. + - It sends `putInstallLlm(token, [])`, refreshes the install session, and advances to GitHub. + - Review screen should show `LLM providers: Skipped`, not `Not configured`, when `session.llm` exists with an empty provider list. + - Keep blank/no-key "Continue" validation unchanged: users cannot accidentally continue with zero providers unless they press the skip action. +- **CLI installer:** Add explicit skip behavior. + - Interactive `fabro install`: before provider selection, ask whether to configure LLM providers now, defaulting to yes. Choosing no returns an empty LLM selection and continues to GitHub. + - Non-interactive `fabro install`: add hidden `--skip-llm`. It is mutually exclusive with `--llm-provider`, `--llm-api-key-stdin`, and `--llm-api-key-env`. + - Keep accidental missing LLM flags as validation errors unless `--skip-llm` is present. + - Update non-interactive usage text to include a skip example. +- **Generated clients/types:** After OpenAPI edit, regenerate/build the API surfaces used by Rust and web: + - `cargo build -p fabro-api` + - `cd lib/packages/fabro-api-client && bun run generate` + +## Test Plan + +- **Server API tests:** Add coverage that `PUT /install/llm` accepts an empty providers list, marks `llm` complete in `/install/session`, and returns `llm.providers: []`. +- **Finish persistence tests:** Add a browser-install finish test with skipped LLMs that asserts: + - `/install/finish` returns `202` + - settings and runtime auth secrets are written + - no LLM credential entries are written to the vault + - GitHub secrets still persist normally +- **Web tests:** Add/update tests for: + - clicking "Skip LLM setup" calls `PUT /install/llm` with `providers: []` and navigates to GitHub + - review screen renders skipped LLMs as `Skipped` + - pressing Continue with no API keys still shows the existing validation error +- **CLI tests:** Add/update unit and integration coverage for: + - `--skip-llm` requires `--non-interactive` + - `--skip-llm` conflicts with all LLM credential flags + - non-interactive validation succeeds with `--skip-llm` plus required GitHub/config flags + - hidden usage text includes `--skip-llm` + - existing no-input non-interactive install still fails +- **Verification commands:** + - `cargo nextest run -p fabro-server -p fabro-cli -p fabro-install` + - `cargo build -p fabro-api` + - `cd apps/fabro-web && bun test` + - `cd apps/fabro-web && bun run typecheck` + +## Assumptions + +- Empty `providers` is the install API representation of an explicit LLM skip; `llm: null` still means the step is incomplete. +- GitHub, server URL, object store, and sandbox setup remain required. +- `fabro doctor` behavior is intentionally unchanged after a skipped LLM install. +- No changes are made to workflow execution or model resolution beyond allowing install to finish without credentials. diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 0ed502b1b..3116d4e0c 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -1561,6 +1561,14 @@ pub(crate) struct InstallNonInteractiveArgs { #[arg(long, hide = true)] pub(crate) llm_api_key_env: Option, + /// Skip LLM setup entirely; install completes with zero LLM credentials + #[arg( + long, + hide = true, + conflicts_with_all = ["llm_provider", "llm_api_key_stdin", "llm_api_key_env"] + )] + pub(crate) skip_llm: bool, + #[arg(long, hide = true)] pub(crate) github_strategy: Option, diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 9d46bd8ec..39ac95d59 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -181,6 +181,7 @@ impl InstallNonInteractiveArgs { self.llm_provider.is_some() || self.llm_api_key_stdin || self.llm_api_key_env.is_some() + || self.skip_llm || self.github_strategy.is_some() || self.github_owner.is_some() || self.github_username.is_some() @@ -196,6 +197,8 @@ impl InstallNonInteractiveArgs { Some("--llm-api-key-stdin") } else if self.llm_api_key_env.is_some() { Some("--llm-api-key-env") + } else if self.skip_llm { + Some("--skip-llm") } else if self.github_strategy.is_some() { Some("--github-strategy") } else if self.github_owner.is_some() { @@ -236,10 +239,16 @@ Non-interactive usage: --github-strategy app \ --github-owner personal + fabro install --non-interactive \ + --skip-llm \ + --github-strategy token \ + --github-username brynary + Hidden non-interactive flags: --llm-provider --llm-api-key-stdin --llm-api-key-env + --skip-llm --github-strategy --github-owner --github-username @@ -249,6 +258,8 @@ Hidden non-interactive flags: Notes: - Only one API-key-based LLM provider is supported in non-interactive mode. + - Pass --skip-llm to finish install without configuring any LLM provider; + it cannot be combined with the --llm-provider or --llm-api-key-* flags. - GitHub App setup prints a local handoff URL and waits for the browser callback."# } @@ -354,6 +365,19 @@ impl InstallInputSource for InteractiveInstallInputSource { s: &Styles, printer: Printer, ) -> Result { + let configure_llm = + spawn_blocking(|| prompt_confirm("Configure LLM providers now?", true)).await??; + if !configure_llm { + fabro_util::printerr!( + printer, + " {} Skipping LLM setup — configure providers later with `fabro provider login`", + s.green.apply_to("✔") + ); + return Ok(LlmInstallSelection { + credentials: Vec::new(), + }); + } + let mut credentials = Vec::new(); let mut configured_providers: Vec = Vec::new(); let mut openai_configured = false; @@ -514,10 +538,14 @@ impl NonInteractiveInstallInputSource { bail!("{}", non_interactive_install_usage()); } - anyhow::ensure!( - args.scripted.llm_api_key_stdin ^ args.scripted.llm_api_key_env.is_some(), - "non-interactive install requires exactly one of --llm-api-key-stdin or --llm-api-key-env" - ); + // `--skip-llm` opts out of LLM setup entirely, so the API-key flags are + // neither required nor allowed (clap enforces the conflict). + if !args.scripted.skip_llm { + anyhow::ensure!( + args.scripted.llm_api_key_stdin ^ args.scripted.llm_api_key_env.is_some(), + "non-interactive install requires exactly one of --llm-api-key-stdin or --llm-api-key-env" + ); + } anyhow::ensure!( !(args.scripted.overwrite_settings && args.scripted.keep_existing_settings), "--overwrite-settings and --keep-existing-settings cannot be used together" @@ -529,10 +557,17 @@ impl NonInteractiveInstallInputSource { } fn validate(&self, config_exists: bool) -> Result<()> { - anyhow::ensure!( - self.args.llm_provider.is_some(), - "non-interactive install requires --llm-provider" - ); + if !self.args.skip_llm && self.args.llm_provider.is_none() { + // Only suggest --skip-llm when no LLM credential flag is present; + // it conflicts with the credential flags, so suggesting it + // alongside one would just send the caller into a conflict error. + let has_api_key_flag = + self.args.llm_api_key_stdin || self.args.llm_api_key_env.is_some(); + if has_api_key_flag { + bail!("non-interactive install requires --llm-provider"); + } + bail!("non-interactive install requires --llm-provider (or --skip-llm)"); + } match self.args.github_strategy { Some(InstallGitHubStrategyArg::Token) => { @@ -599,6 +634,11 @@ impl InstallInputSource for NonInteractiveInstallInputSource { s: &Styles, printer: Printer, ) -> Result { + if self.args.skip_llm { + return Ok(LlmInstallSelection { + credentials: Vec::new(), + }); + } let provider = self .args .llm_provider @@ -3044,6 +3084,66 @@ root = "{}" ); } + #[test] + fn non_interactive_source_accepts_skip_llm_without_credential_flags() { + let args = install_args(true, InstallNonInteractiveArgs { + skip_llm: true, + github_strategy: Some(InstallGitHubStrategyArg::Token), + github_username: Some("brynary".to_string()), + ..InstallNonInteractiveArgs::default() + }); + + // `--skip-llm` alone is enough scripted input; the API-key flags are + // neither required nor allowed when skipping LLM setup. + NonInteractiveInstallInputSource::new(&args) + .unwrap() + .expect("--skip-llm should be accepted as non-interactive input"); + } + + #[test] + fn non_interactive_source_validate_allows_skip_llm_without_provider() { + let source = NonInteractiveInstallInputSource { + args: InstallNonInteractiveArgs { + skip_llm: true, + github_strategy: Some(InstallGitHubStrategyArg::Token), + github_username: Some("brynary".to_string()), + ..InstallNonInteractiveArgs::default() + }, + }; + + source.validate(false).unwrap(); + } + + #[tokio::test] + async fn non_interactive_source_skip_llm_collects_no_credentials() { + let source = NonInteractiveInstallInputSource { + args: InstallNonInteractiveArgs { + skip_llm: true, + github_strategy: Some(InstallGitHubStrategyArg::Token), + github_username: Some("brynary".to_string()), + ..InstallNonInteractiveArgs::default() + }, + }; + + let facts = InstallFacts { + codex_detected: false, + }; + let selection = source + .collect_llm_selection(&facts, &Styles::detect_stderr(), Printer::Silent) + .await + .unwrap(); + assert!( + selection.credentials.is_empty(), + "--skip-llm should collect zero LLM credentials" + ); + } + + #[test] + fn non_interactive_install_usage_documents_skip_llm() { + let usage = non_interactive_install_usage(); + assert!(usage.contains("--skip-llm")); + } + #[test] fn non_interactive_source_rejects_missing_github_strategy() { let source = NonInteractiveInstallInputSource { diff --git a/lib/crates/fabro-cli/tests/it/cmd/install.rs b/lib/crates/fabro-cli/tests/it/cmd/install.rs index b4b3fcb1d..cecc043ed 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/install.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/install.rs @@ -180,6 +180,78 @@ fn hidden_non_interactive_args_require_non_interactive() { assert!(stderr.contains("requires --non-interactive")); } +#[test] +fn skip_llm_requires_non_interactive() { + let context = test_context!(); + let output = context + .command() + .args(["install", "--skip-llm"]) + .output() + .expect("command should run"); + + assert!(!output.status.success()); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!(stderr.contains("--skip-llm requires --non-interactive")); +} + +#[test] +fn skip_llm_conflicts_with_llm_credential_flags() { + let context = test_context!(); + + let provider_conflict = context + .command() + .args([ + "install", + "--non-interactive", + "--skip-llm", + "--llm-provider", + "anthropic", + ]) + .output() + .expect("command should run"); + assert!(!provider_conflict.status.success()); + let stderr = String::from_utf8(provider_conflict.stderr).unwrap(); + assert!( + stderr.contains("--skip-llm") && stderr.contains("--llm-provider"), + "expected a conflict error between --skip-llm and --llm-provider: {stderr}" + ); + + let stdin_conflict = context + .command() + .args([ + "install", + "--non-interactive", + "--skip-llm", + "--llm-api-key-stdin", + ]) + .output() + .expect("command should run"); + assert!(!stdin_conflict.status.success()); + let stderr = String::from_utf8(stdin_conflict.stderr).unwrap(); + assert!( + stderr.contains("--skip-llm") && stderr.contains("--llm-api-key-stdin"), + "expected a conflict error between --skip-llm and --llm-api-key-stdin: {stderr}" + ); + + let env_conflict = context + .command() + .args([ + "install", + "--non-interactive", + "--skip-llm", + "--llm-api-key-env", + "ANTHROPIC_API_KEY", + ]) + .output() + .expect("command should run"); + assert!(!env_conflict.status.success()); + let stderr = String::from_utf8(env_conflict.stderr).unwrap(); + assert!( + stderr.contains("--skip-llm") && stderr.contains("--llm-api-key-env"), + "expected a conflict error between --skip-llm and --llm-api-key-env: {stderr}" + ); +} + #[test] fn github_requires_prior_install() { let context = test_context!(); diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index a79c629df..32e7a5e9d 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -800,14 +800,9 @@ async fn put_install_llm( } observe_operator(&state, &headers); - if input.providers.is_empty() { - return ( - StatusCode::UNPROCESSABLE_ENTITY, - Json(serde_json::json!({ "error": "at least one LLM provider is required" })), - ) - .into_response(); - } - + // An empty providers list is an explicit skip: the LLM step is recorded as + // completed with zero credentials. `/install/finish` still requires the + // step to be present, just not populated. for provider in &input.providers { if let Some(error) = unsupported_install_provider_error(provider.provider) { return install_error_response(StatusCode::UNPROCESSABLE_ENTITY, error); diff --git a/lib/crates/fabro-server/tests/it/api/install.rs b/lib/crates/fabro-server/tests/it/api/install.rs index 08433daac..153342491 100644 --- a/lib/crates/fabro-server/tests/it/api/install.rs +++ b/lib/crates/fabro-server/tests/it/api/install.rs @@ -162,6 +162,23 @@ async fn put_install_llm(app: &axum::Router, token: &str) { response_status(response, StatusCode::NO_CONTENT, "PUT /install/llm").await; } +async fn put_install_llm_skipped(app: &axum::Router, token: &str) { + let response = app + .clone() + .oneshot( + Request::builder() + .method("PUT") + .uri("/install/llm") + .header("authorization", format!("Bearer {token}")) + .header("content-type", "application/json") + .body(Body::from(r#"{"providers":[]}"#)) + .expect("skipped LLM install request should build"), + ) + .await + .unwrap(); + response_status(response, StatusCode::NO_CONTENT, "PUT /install/llm").await; +} + async fn put_install_github_token(app: &axum::Router, token: &str, username: &str) { let response = app .clone() @@ -884,6 +901,99 @@ async fn token_install_finish_persists_settings_env_and_vault() { assert_eq!(vault.get("GITHUB_TOKEN"), Some("ghp_test_token")); } +#[tokio::test] +async fn install_llm_accepts_empty_providers_as_explicit_skip() { + let app = build_install_router(InstallAppState::for_test("test-install-token")); + + put_install_llm_skipped(&app, "test-install-token").await; + + let session_response = app + .oneshot( + Request::builder() + .method("GET") + .uri("/install/session") + .header("authorization", "Bearer test-install-token") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let session_body = + response_json(session_response, StatusCode::OK, "GET /install/session").await; + assert!( + session_body["completed_steps"] + .as_array() + .unwrap() + .iter() + .any(|value| value == "llm"), + "skipped LLM step should still count as completed" + ); + assert_eq!( + session_body["llm"]["providers"], + serde_json::json!([]), + "skipped LLM step should expose an empty providers list" + ); +} + +#[tokio::test] +async fn browser_install_finish_with_skipped_llm_persists_no_llm_credentials() { + let temp_dir = tempfile::tempdir().unwrap(); + let config_path = temp_dir.path().join("settings.toml"); + let app = build_install_router(InstallAppState::for_test_with_paths( + "test-install-token", + temp_dir.path(), + &config_path, + )); + put_install_server(&app, "test-install-token", "https://fabro.example.com").await; + put_install_object_store_local(&app, "test-install-token").await; + put_install_sandbox_docker(&app, "test-install-token").await; + put_install_llm_skipped(&app, "test-install-token").await; + put_install_github_token(&app, "test-install-token", "brynary").await; + + let finish_response = app + .oneshot( + Request::builder() + .method("POST") + .uri("/install/finish") + .header("authorization", "Bearer test-install-token") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let finish_body = response_json( + finish_response, + StatusCode::ACCEPTED, + "POST /install/finish", + ) + .await; + assert_eq!(finish_body["status"], "completing"); + + let settings = std::fs::read_to_string(&config_path).unwrap(); + assert!(settings.contains("https://fabro.example.com")); + assert!(settings.contains("strategy = \"token\"")); + + let server_env = std::fs::read_to_string( + fabro_config::Storage::new(temp_dir.path()) + .runtime_directory() + .env_path(), + ) + .unwrap(); + assert!(server_env.contains("SESSION_SECRET=")); + assert!(server_env.contains("FABRO_DEV_TOKEN=")); + + let vault = Vault::load(fabro_config::Storage::new(temp_dir.path()).secrets_path()).unwrap(); + assert!( + vault.credential_entries().is_empty(), + "skipped LLM install should not write any credential vault entries" + ); + assert_eq!( + vault.get("GITHUB_TOKEN"), + Some("ghp_test_token"), + "GitHub secrets still persist when the LLM step is skipped" + ); +} + #[tokio::test] async fn token_install_finish_invokes_finish_hook_before_response_returns() { let temp_dir = tempfile::tempdir().unwrap(); diff --git a/lib/packages/fabro-api-client/src/api/install-api.ts b/lib/packages/fabro-api-client/src/api/install-api.ts index 820a5dc3a..f0218bc66 100644 --- a/lib/packages/fabro-api-client/src/api/install-api.ts +++ b/lib/packages/fabro-api-client/src/api/install-api.ts @@ -233,7 +233,7 @@ export const InstallApiAxiosParamCreator = function (configuration?: Configurati }; }, /** - * Records the LLM providers and API keys chosen during the browser install. Requires the one-time install token. + * Records the LLM providers and API keys chosen during the browser install. An empty `providers` list marks the LLM step as completed and explicitly skipped. Requires the one-time install token. * @summary Save install LLM settings * @param {InstallLlmProvidersInput} installLlmProvidersInput * @param {*} [options] Override http request option. @@ -586,7 +586,7 @@ export const InstallApiFp = function(configuration?: Configuration) { return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); }, /** - * Records the LLM providers and API keys chosen during the browser install. Requires the one-time install token. + * Records the LLM providers and API keys chosen during the browser install. An empty `providers` list marks the LLM step as completed and explicitly skipped. Requires the one-time install token. * @summary Save install LLM settings * @param {InstallLlmProvidersInput} installLlmProvidersInput * @param {*} [options] Override http request option. @@ -748,7 +748,7 @@ export const InstallApiFactory = function (configuration?: Configuration, basePa return localVarFp.putInstallGithubToken(installGithubTokenInput, options).then((request) => request(axios, basePath)); }, /** - * Records the LLM providers and API keys chosen during the browser install. Requires the one-time install token. + * Records the LLM providers and API keys chosen during the browser install. An empty `providers` list marks the LLM step as completed and explicitly skipped. Requires the one-time install token. * @summary Save install LLM settings * @param {InstallLlmProvidersInput} installLlmProvidersInput * @param {*} [options] Override http request option. @@ -889,7 +889,7 @@ export class InstallApi extends BaseAPI { } /** - * Records the LLM providers and API keys chosen during the browser install. Requires the one-time install token. + * Records the LLM providers and API keys chosen during the browser install. An empty `providers` list marks the LLM step as completed and explicitly skipped. Requires the one-time install token. * @summary Save install LLM settings * @param {InstallLlmProvidersInput} installLlmProvidersInput * @param {*} [options] Override http request option. diff --git a/lib/packages/fabro-api-client/src/models/install-llm-providers-input.ts b/lib/packages/fabro-api-client/src/models/install-llm-providers-input.ts index 3c3c74094..70d704f56 100644 --- a/lib/packages/fabro-api-client/src/models/install-llm-providers-input.ts +++ b/lib/packages/fabro-api-client/src/models/install-llm-providers-input.ts @@ -18,9 +18,12 @@ import type { InstallLlmProviderInput } from './install-llm-provider-input'; /** - * LLM providers selected during browser install. + * LLM providers selected during browser install. An empty `providers` list explicitly marks the LLM step as completed and skipped. */ export interface InstallLlmProvidersInput { + /** + * LLM providers to persist. An empty list records an explicit skip: the LLM step is marked complete with zero credentials. + */ 'providers': Array; } diff --git a/lib/packages/fabro-api-client/src/models/install-llm-summary.ts b/lib/packages/fabro-api-client/src/models/install-llm-summary.ts index a70d24005..4b8ce3b60 100644 --- a/lib/packages/fabro-api-client/src/models/install-llm-summary.ts +++ b/lib/packages/fabro-api-client/src/models/install-llm-summary.ts @@ -18,7 +18,7 @@ import type { InstallLlmSummaryProvidersInner } from './install-llm-summary-providers-inner'; /** - * Redacted summary of persisted LLM install choices. + * Redacted summary of persisted LLM install choices. Present with an empty `providers` list when the LLM step was explicitly skipped; `null` on the install session means the step is still incomplete. */ export interface InstallLlmSummary { 'providers'?: Array;