From 7b8a6f5cceabddf7a68cc66014ef0ec6eba3e96e Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Thu, 11 Jun 2026 19:32:51 -0400 Subject: [PATCH] fix(cliproxy): close second-round review gaps in pool onboarding and remote env - create-command: show the pool onboarding hint only after profile creation fully succeeds; the pre-create placement burned the once-per-install dismissal when creation failed or rolled back - remote env (claude): skip the read-level stale-pin filter once the migration marker exists - a post-migration pin is user-intentional (e.g. an explicit --config pick equal to a historical default) - unified-config loader: document that loadOrCreateUnifiedConfig never writes to disk, so read paths on legacy installs stay side-effect free --- src/auth/commands/create-command.ts | 22 ++++++------ .../__tests__/pool-onboarding-phase5.test.ts | 20 +++++------ .../__tests__/claude-model-neutral.test.ts | 34 +++++++++++++++++++ src/cliproxy/config/env-builder.ts | 12 ++++--- src/config/unified-config-loader.ts | 3 ++ 5 files changed, 64 insertions(+), 27 deletions(-) diff --git a/src/auth/commands/create-command.ts b/src/auth/commands/create-command.ts index 262dc838..05eacc4a 100644 --- a/src/auth/commands/create-command.ts +++ b/src/auth/commands/create-command.ts @@ -189,18 +189,6 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise } }; - // Pool suggestion: when creating a 2nd+ native Claude profile, show the - // once-per-install onboarding hint. Print-only, TTY-gated, never blocks. - // Pass existingCount + 1 so the hint sees the post-create count (>= 2). - // Gated on hasUnifiedConfig() so legacy profiles.json-only installs receive - // the hint from ccs doctor only (where dismissal semantics are preserved). - if (!profileExistedBeforeCreate && hasUnifiedConfig()) { - const existingCount = countNativeClaudeProfiles(); - if (existingCount >= 1) { - maybeShowPoolOnboardingHint(existingCount + 1); - } - } - try { // Create instance directory console.log(info(`Creating profile: ${profileName}`)); @@ -326,6 +314,16 @@ export async function handleCreate(ctx: CommandContext, args: string[]): Promise ) ); console.log(''); + // Pool suggestion: shown only AFTER the profile creation fully + // succeeded (a pre-create hint would burn the once-per-install + // dismissal even when creation fails or rolls back). Print-only, + // TTY-gated, never blocks. Gated on hasUnifiedConfig() so legacy + // profiles.json-only installs receive the hint from ccs doctor only + // (where dismissal semantics are preserved). The profile now exists, + // so the registry count is already post-create (no +1 needed). + if (!profileExistedBeforeCreate && hasUnifiedConfig()) { + maybeShowPoolOnboardingHint(countNativeClaudeProfiles()); + } process.exit(0); } else { await rollbackFailedCreate(); diff --git a/src/cliproxy/__tests__/pool-onboarding-phase5.test.ts b/src/cliproxy/__tests__/pool-onboarding-phase5.test.ts index bf8ab6e2..b0001ee9 100644 --- a/src/cliproxy/__tests__/pool-onboarding-phase5.test.ts +++ b/src/cliproxy/__tests__/pool-onboarding-phase5.test.ts @@ -320,25 +320,23 @@ describe('Phase 5: Pool Onboarding Hint', () => { }); // ── 11. Create-command suggestion: hint fires with post-create count ────── - it('hint fires with count 2 when create-command passes existingCount+1 for 1 existing profile', async () => { + it('hint fires with count 2 after a successful 2nd-profile create (post-create call site)', async () => { // NOTE: SIMULATION of the create-command call site, not the real command. - // It replicates the threshold logic (existingCount + 1) and calls the same - // exported hint symbol; it does not invoke create-command end to end. - // 1 existing profile in the registry, simulating pre-create state. - // create-command calls maybeShowPoolOnboardingHint(existingCount + 1) = hint(2). + // create-command shows the hint only AFTER profile creation succeeds (a + // pre-create hint would burn the once-per-install dismissal on a failed + // create). At that point the registry already contains the new profile, + // so the call is maybeShowPoolOnboardingHint(countNativeClaudeProfiles()). const ccsDir = path.join(tempHome, '.ccs'); - writeProfiles(ccsDir, ['work']); + writeProfiles(ccsDir, ['work', 'personal']); // post-create state: 2 profiles const consoleSpy = spyOn(console, 'log').mockImplementation(() => {}); try { const { maybeShowPoolOnboardingHint, countNativeClaudeProfiles } = await import( `../routing/pool-onboarding-hint?p5create=${Date.now()}` ); - // Replicate the threshold logic from create-command.ts lines 195-200 - const existingCount = countNativeClaudeProfiles(); - expect(existingCount).toBe(1); - // existingCount >= 1 so the hint is called with existingCount + 1 - const result = maybeShowPoolOnboardingHint(existingCount + 1); + const count = countNativeClaudeProfiles(); + expect(count).toBe(2); + const result = maybeShowPoolOnboardingHint(count); expect(result.printed).toBe(true); const allOutput = consoleSpy.mock.calls.map((c) => String(c[0])).join('\n'); expect(allOutput).toContain('2 Claude profiles'); diff --git a/src/cliproxy/config/__tests__/claude-model-neutral.test.ts b/src/cliproxy/config/__tests__/claude-model-neutral.test.ts index 2c002b5e..b9338b9c 100644 --- a/src/cliproxy/config/__tests__/claude-model-neutral.test.ts +++ b/src/cliproxy/config/__tests__/claude-model-neutral.test.ts @@ -550,4 +550,38 @@ describe('claude provider model-neutral passthrough (Gap 1)', () => { // Priority 1 untouched: the explicit pin survives. expect(env.ANTHROPIC_MODEL).toBe('claude-sonnet-4-6'); }); + + it('getRemoteEnvVars (claude): migration marker set means pins are user-intentional and NOT filtered', () => { + process.env.CCS_HOME = tempHome; + const ccsDir = path.join(tempHome, '.ccs'); + fs.mkdirSync(ccsDir, { recursive: true }); + + // Marker present: the file was already cleaned once, so any pin that + // exists now was put there deliberately (e.g. ccs claude --config picked + // a value that happens to equal a historical default). + const markerDir = path.join(ccsDir, 'cliproxy'); + fs.mkdirSync(markerDir, { recursive: true }); + fs.writeFileSync(path.join(markerDir, '.claude-model-migrated'), new Date().toISOString()); + + const settingsPath = path.join(ccsDir, 'claude.settings.json'); + fs.writeFileSync( + settingsPath, + JSON.stringify({ + env: { + ANTHROPIC_BASE_URL: 'http://127.0.0.1:8317/api/provider/claude', + ANTHROPIC_AUTH_TOKEN: 'ccs-internal-managed', + ANTHROPIC_MODEL: 'claude-sonnet-4-6', // equals a stale default, but post-migration = explicit + }, + }), + 'utf-8' + ); + + const env = getRemoteEnvVars('claude', { + host: 'example.com', + port: 8317, + protocol: 'http', + }); + + expect(env.ANTHROPIC_MODEL).toBe('claude-sonnet-4-6'); + }); }); diff --git a/src/cliproxy/config/env-builder.ts b/src/cliproxy/config/env-builder.ts index 5d47dae0..998a10f1 100644 --- a/src/cliproxy/config/env-builder.ts +++ b/src/cliproxy/config/env-builder.ts @@ -808,10 +808,14 @@ export function getRemoteEnvVars( // never rewrites the file, so a remote-only user whose settings still // carry an old auto-written default would stay pinned. Filter stale // defaults at read level so values equal to a historical default are - // dropped while user-custom pins survive. Priority 1 (explicit custom - // settings path) is intentionally left untouched: an explicitly passed - // settings file is the user's deliberate choice. - if (provider === 'claude') { + // dropped while user-custom pins survive. Once the migration marker + // exists the file has already been cleaned, so any pin present after + // that is user-intentional (e.g. an explicit `ccs claude --config` + // pick that happens to equal a historical default) and must NOT be + // filtered. Priority 1 (explicit custom settings path) is + // intentionally left untouched: an explicitly passed settings file + // is the user's deliberate choice. + if (provider === 'claude' && !claudeModelMigrationDone()) { userEnvVars = filterClaudeStaleModelPins(userEnvVars); } } diff --git a/src/config/unified-config-loader.ts b/src/config/unified-config-loader.ts index fcd943bd..e7f8c87a 100644 --- a/src/config/unified-config-loader.ts +++ b/src/config/unified-config-loader.ts @@ -191,6 +191,9 @@ export function loadUnifiedConfig(): UnifiedConfig | null { * Merges with defaults to ensure all sections exist. */ export function loadOrCreateUnifiedConfig(): UnifiedConfig { + // Read-only: "create" means an in-memory default object when config.yaml is + // absent. This never writes to disk, so callers on legacy installs can use + // it for read paths without implicitly creating config.yaml. const existing = loadUnifiedConfig(); if (existing) { const merged = mergeWithDefaults(existing);