Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/__tests__/bugfixes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -695,3 +695,37 @@ describe('Dev-channels dialog coverage', () => {
)
})
})

// ---------------------------------------------------------------------------
// Fix: onboarding + trust dialog skipped entirely for third-party providers
// ---------------------------------------------------------------------------
// Behavioral coverage lives in src/utils/setupScreenGates.test.ts — the
// gating decisions were extracted into that provider-free seam because this
// module's import chain cannot be loaded under bun test (compile-time
// feature() macro checker, same constraint as the dev-channels tests above).
// These wiring checks assert showSetupScreens actually consults the seam and
// that no provider gate was re-introduced around the dialogs.
describe('Onboarding and trust dialog — third-party providers', () => {
test('showSetupScreens routes both dialogs through the provider-free seam', async () => {
const content = await file('interactiveHelpers.tsx').text()

expect(content).toContain('getRequiredSetupScreens({')
expect(content).toContain('if (setupScreens.onboarding)')
expect(content).toContain('if (setupScreens.trustDialog)')
})

test('no dialog is gated behind usesAnthropicSetup', async () => {
const content = await file('interactiveHelpers.tsx').text()

// Theme choice + security notes are universal, and workspace trust is
// orthogonal to the API provider: an untrusted repo is exactly as
// dangerous over a local model as over Anthropic. The seam takes no
// provider input, so the only way to regress is to add a gate at the
// call sites — which this guards against.
expect(content).not.toMatch(/usesAnthropicSetup\s*&&\s*\(?\s*setupScreens/)
expect(content).not.toMatch(/usesAnthropicSetup\s*&&\s*\(\s*!config\.theme/)
expect(content).not.toMatch(
/usesAnthropicSetup\s*&&\s*!checkHasTrustDialogAccepted/,
)
})
})
Comment on lines +559 to +749

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Tests assert source-text shape, not actual runtime behavior.

Both new tests regex-match the raw text of interactiveHelpers.tsx rather than exercising showSetupScreens itself. They'll pass as long as the literal string usesAnthropicSetup && ... isn't present, regardless of whether the dialogs actually render correctly for third-party providers (or regress via an equivalent-but-differently-worded condition). As per path instructions, "Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior" — this change to startup dialog gating for all providers is exactly this kind of risky runtime change.

Consider adding a behavioral test that mocks usesAnthropicAccountFlow() to return false and checkHasTrustDialogAccepted() to return false, then asserts showSetupScreens actually invokes the onboarding/trust dialog imports/renderers for a non-Anthropic provider.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/__tests__/bugfixes.test.ts` around lines 552 - 579, The new coverage in
interactiveHelpers.tsx is asserting source text shape instead of the runtime
behavior of showSetupScreens, so it can miss regressions in actual dialog
rendering for non-Anthropic providers. Add a behavioral test around
showSetupScreens that mocks usesAnthropicAccountFlow() to false and
checkHasTrustDialogAccepted() to false, then verifies the onboarding and trust
dialog paths are actually invoked for third-party providers. Keep the existing
regex checks only as supplemental coverage, not the primary assertion.

Source: Path instructions

67 changes: 67 additions & 0 deletions src/components/ConsoleOAuthFlow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ import { sendNotification } from '../services/notifier.js';
import { OAuthService } from '../services/oauth/index.js';
import { getOauthAccountInfo, validateForceLoginOrg } from '../utils/auth.js';
import { logError } from '../utils/log.js';
import { getLocalOpenAICompatibleProviderLabel } from '../utils/providerDiscovery.js';
import {
addProviderProfile,
getProviderProfiles,
setActiveProviderProfile,
} from '../utils/providerProfiles.js';
import { getSettings_DEPRECATED } from '../utils/settings/settings.js';
import { ProviderManager } from './ProviderManager.js';
import { Select } from './CustomSelect/select.js';
Expand Down Expand Up @@ -386,7 +392,33 @@ function OAuthStatusMessage({
startingMessage ||
'OpenClaude can be used with your Claude subscription or billed based on API usage through your Console account.'

// OPENAI_BASE_URL/OPENAI_MODEL in the environment signal an
// OpenAI-compatible setup the user already has — offer to adopt it as
// the active provider profile instead of walking them through login for
// an account they may never have wanted. Env vars alone do NOT activate
// the route (resolveActiveRouteIdFromEnv requires CLAUDE_CODE_USE_OPENAI
// or a saved profile), so selecting this saves + activates a profile.
// Both fields gate the option because a profile requires baseUrl+model.
const envBaseUrl =
process.env.OPENAI_BASE_URL ?? process.env.OPENAI_API_BASE
const envModel = process.env.OPENAI_MODEL
const envConfigAvailable = Boolean(envBaseUrl && envModel)

const loginOptions = [
...(envConfigAvailable
? [
{
label: (
<Text>
Use current environment configuration ·{' '}
<Text dimColor>OPENAI_BASE_URL={envBaseUrl}</Text>
{'\n'}
</Text>
),
value: 'environment' as const,
},
]
: []),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
{
label: (
<Text>
Expand Down Expand Up @@ -427,6 +459,41 @@ function OAuthStatusMessage({
<Select
options={loginOptions}
onChange={value => {
if (value === 'environment') {
// Re-entering this flow with the same env must not stack
// near-identical profiles: reuse (and activate) an existing
// profile matching the env base URL + model before creating
// a new one. setActiveProviderProfile also re-applies the
// profile env and syncs the startup profile file.
const existing = getProviderProfiles().find(
profile =>
profile.baseUrl?.trim() === envBaseUrl?.trim() &&
profile.model?.trim() === envModel?.trim(),
)
const saved = existing
? setActiveProviderProfile(existing.id)
: addProviderProfile(
{
name: getLocalOpenAICompatibleProviderLabel(envBaseUrl),
baseUrl: envBaseUrl as string,
model: envModel as string,
apiKey: process.env.OPENAI_API_KEY,
},
{ makeActive: true },
)
if (!saved) {
// Env values failed profile validation — fall back to the
// guided provider setup with fields prefilled from env.
setOAuthStatus({ state: 'platform_setup' })
return
}
logEvent('tengu_oauth_env_config_selected', {})
setOAuthStatus({
state: 'platform_setup_complete',
message: `Saved ${saved.name} (${envBaseUrl}) as your active provider.`,
})
return
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (value === 'platform') {
logEvent('tengu_oauth_platform_selected', {})
setOAuthStatus({ state: 'platform_setup' })
Expand Down
26 changes: 20 additions & 6 deletions src/interactiveHelpers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { onChangeAppState } from './state/onChangeAppState.js';
import { normalizeApiKeyForConfig } from './utils/authPortable.js';
import { getExternalClaudeMdIncludes, getMemoryFiles, shouldShowClaudeMdExternalIncludesWarning } from './utils/claudemd.js';
import { checkHasTrustDialogAccepted, getCustomApiKeyStatus, getGlobalConfig, saveGlobalConfig } from './utils/config.js';
import { getRequiredSetupScreens } from './utils/setupScreenGates.js';
import { updateDeepLinkTerminalPreference } from './utils/deepLink/terminalPreference.js';
import { isEnvTruthy, isRunningOnHomespace } from './utils/envUtils.js';
import { type FpsMetrics, FpsTracker } from './utils/fpsTracker.js';
Expand Down Expand Up @@ -114,9 +115,20 @@ export async function showSetupScreens(root: Root, permissionMode: PermissionMod
const config = getGlobalConfig();
let onboardingShown = false;

// Skip onboarding dialog for third-party providers (no Anthropic account needed)
if (usesAnthropicSetup && (!config.theme || !config.hasCompletedOnboarding) // always show onboarding at least once
) {
// Onboarding runs for ALL providers: theme + security notes are universal,
// and the component itself drops the preflight/OAuth steps when Anthropic
// auth is not enabled (see oauthEnabled in Onboarding.tsx). Gating this on
// the Anthropic account flow left third-party users with no theme choice
// and, worse, no prompt-injection/safety notes. The decisions live in the
// provider-free setupScreenGates seam (behaviorally tested there — this
// module's import chain cannot be loaded under bun test).
const setupScreens = getRequiredSetupScreens({
theme: config.theme,
hasCompletedOnboarding: config.hasCompletedOnboarding,
trustDialogAccepted: checkHasTrustDialogAccepted(),
isClaubbit: isEnvTruthy(process.env.CLAUBBIT),
});
if (setupScreens.onboarding) {
onboardingShown = true;
const {
Onboarding
Expand All @@ -136,9 +148,11 @@ export async function showSetupScreens(root: Root, permissionMode: PermissionMod
// Note: non-interactive sessions (CI/CD with -p) never reach showSetupScreens at all.
// Skip permission checks in claubbit
if (!isEnvTruthy(process.env.CLAUBBIT)) {
// Skip trust dialog UI for third-party providers (no Anthropic auth), but still
// run trust state initialization below so the REPL mounts correctly.
if (usesAnthropicSetup && !checkHasTrustDialogAccepted()) {
// The trust dialog is the workspace trust boundary — it has nothing to do
// with which API provider is configured, so it runs for third-party
// providers too (an untrusted repo is exactly as dangerous over Ollama as
// over Anthropic).
if (setupScreens.trustDialog) {
const {
TrustDialog
} = await import('./components/TrustDialog/TrustDialog.js');
Expand Down
69 changes: 69 additions & 0 deletions src/utils/setupScreenGates.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { describe, expect, test } from 'bun:test'

import { getRequiredSetupScreens } from './setupScreenGates.js'

// Behavioral coverage for the first-run screen gating (#1864). The seam is
// deliberately provider-free — there is no input to vary by provider, which
// IS the fix: a third-party (or any) provider gets the same onboarding and
// workspace-trust decisions as the Anthropic account flow.
// showSetupScreens itself cannot be imported under bun test (its import
// chain trips the compile-time feature() macro checker), so the wiring is
// asserted structurally in src/__tests__/bugfixes.test.ts.

describe('getRequiredSetupScreens', () => {
const completed = {
theme: 'dark',
hasCompletedOnboarding: true,
trustDialogAccepted: true,
isClaubbit: false,
}

test('fresh install shows both screens', () => {
expect(
getRequiredSetupScreens({
theme: undefined,
hasCompletedOnboarding: undefined,
trustDialogAccepted: false,
isClaubbit: false,
}),
).toEqual({ onboarding: true, trustDialog: true })
})

test('fully set-up install shows neither', () => {
expect(getRequiredSetupScreens(completed)).toEqual({
onboarding: false,
trustDialog: false,
})
})

test('onboarding re-shows when the theme is missing even if completed once', () => {
expect(
getRequiredSetupScreens({ ...completed, theme: undefined }).onboarding,
).toBe(true)
})

test('onboarding re-shows when never completed even with a theme set', () => {
expect(
getRequiredSetupScreens({ ...completed, hasCompletedOnboarding: false })
.onboarding,
).toBe(true)
})

test('trust dialog shows whenever unaccepted, independent of onboarding state', () => {
expect(
getRequiredSetupScreens({ ...completed, trustDialogAccepted: false })
.trustDialog,
).toBe(true)
})

test('claubbit skips the trust dialog but never onboarding', () => {
const result = getRequiredSetupScreens({
theme: undefined,
hasCompletedOnboarding: false,
trustDialogAccepted: false,
isClaubbit: true,
})
expect(result.trustDialog).toBe(false)
expect(result.onboarding).toBe(true)
})
})
29 changes: 29 additions & 0 deletions src/utils/setupScreenGates.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* Pure gating decisions for the first-run setup screens, extracted from
* showSetupScreens (interactiveHelpers.tsx) as an importable seam:
* interactiveHelpers cannot be imported in tests — its import chain trips
* Bun's compile-time feature() macro checker before mocks can intercept —
* so behavioral coverage lives against this module instead (the same pattern
* as the dev-channels registration seam).
*
* Deliberately provider-free: NO input carries which API provider is active.
* That absence is the fix (#1864) — onboarding (theme + safety notes) is
* universal, with Onboarding.tsx itself dropping the OAuth/preflight steps
* when Anthropic auth is off, and workspace trust is exactly as load-bearing
* over a local model as over Anthropic. Re-introducing a provider parameter
* here should be treated as a regression signal in review.
*/
export function getRequiredSetupScreens(options: {
theme: string | undefined
hasCompletedOnboarding: boolean | undefined
trustDialogAccepted: boolean
isClaubbit: boolean
}): { onboarding: boolean; trustDialog: boolean } {
return {
// Always show onboarding at least once (theme unset or never completed).
onboarding: !options.theme || !options.hasCompletedOnboarding,
// The trust dialog is the workspace trust boundary; only the claubbit
// harness (which owns its own trust story) skips it.
trustDialog: !options.isClaubbit && !options.trustDialogAccepted,
}
}