Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
25 changes: 25 additions & 0 deletions src/browser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,31 @@ describe('BrowserBridge state', () => {
await expect(bridge.connect({ timeout: 0.1 })).rejects.toThrow('Browser Bridge extension not connected');
});

it('threads preferredContextId into every readiness health read', async () => {
const { PKG_VERSION } = await import('./version.js');
const spy = vi.spyOn(daemonTransport, 'getDaemonHealth').mockResolvedValue({
state: 'no-extension',
status: {
ok: true,
pid: 999999,
uptime: 0,
daemonVersion: PKG_VERSION,
extensionConnected: false,
pending: 0,
memoryMB: 0,
port: 0,
},
});

const bridge = new BrowserBridge();

await expect(bridge.connect({ timeout: 0.1, preferredContextId: 'zvypsyje' })).rejects.toThrow('Browser Bridge extension not connected');
expect(spy.mock.calls.length).toBeGreaterThan(1);
for (const call of spy.mock.calls) {
expect(call[0]).toMatchObject({ preferredContextId: 'zvypsyje' });
}
});

it('attempts stale daemon replacement when daemonVersion is missing', async () => {
vi.spyOn(daemonTransport, 'getDaemonHealth').mockResolvedValue({
state: 'no-extension',
Expand Down
13 changes: 13 additions & 0 deletions src/browser/bridge-readiness.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,19 @@ describe('waitForBridgeReady', () => {
expect(fetchHealth).toHaveBeenCalledTimes(3);
});

it('forwards preferredContextId to every health poll', async () => {
const sequence: DaemonHealth[] = [notReadyHealth('profile-required'), readyHealth()];
let i = 0;
const fetchHealth: HealthFetcher = vi.fn(async () => sequence[i++] ?? readyHealth());

await waitForBridgeReady(fetchHealth, { timeoutMs: 10_000, intervalMs: 1, preferredContextId: 'zvypsyje' });

expect(vi.mocked(fetchHealth).mock.calls.length).toBeGreaterThan(1);
for (const call of vi.mocked(fetchHealth).mock.calls) {
expect(call[0]).toMatchObject({ preferredContextId: 'zvypsyje' });
}
});

it('returns the last observed non-ready health when the deadline expires', async () => {
const fetchHealth: HealthFetcher = vi.fn(async () => notReadyHealth('profile-disconnected'));

Expand Down
8 changes: 4 additions & 4 deletions src/browser/bridge-readiness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,22 @@ import type { DaemonHealth } from './daemon-transport.js';

export type { DaemonHealth };

export type HealthFetcher = (opts?: { timeout?: number; contextId?: string }) => Promise<DaemonHealth>;
export type HealthFetcher = (opts?: { timeout?: number; contextId?: string; preferredContextId?: string }) => Promise<DaemonHealth>;

const DEFAULT_POLL_INTERVAL_MS = 200;

export async function waitForBridgeReady(
fetchHealth: HealthFetcher,
opts: { timeoutMs: number; contextId?: string; intervalMs?: number },
opts: { timeoutMs: number; contextId?: string; preferredContextId?: string; intervalMs?: number },
): Promise<DaemonHealth> {
const interval = opts.intervalMs ?? DEFAULT_POLL_INTERVAL_MS;
let health = await fetchHealth({ contextId: opts.contextId });
let health = await fetchHealth({ contextId: opts.contextId, preferredContextId: opts.preferredContextId });
if (health.state === 'ready') return health;

const deadline = Date.now() + opts.timeoutMs;
while (Date.now() < deadline) {
await new Promise<void>((resolve) => setTimeout(resolve, interval));
health = await fetchHealth({ contextId: opts.contextId });
health = await fetchHealth({ contextId: opts.contextId, preferredContextId: opts.preferredContextId });
if (health.state === 'ready') return health;
}
return health;
Expand Down
5 changes: 3 additions & 2 deletions src/browser/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export class BrowserBridge implements IBrowserFactory {
const routing = opts.contextId || opts.preferredContextId
? { contextId: opts.contextId, preferredContextId: opts.preferredContextId }
: profileRouteParams(resolveProfileSelection());
await this._ensureDaemon(opts.timeout, routing.contextId);
await this._ensureDaemon(opts.timeout, routing.contextId, routing.preferredContextId);
if (!opts.session?.trim()) throw new Error('Browser session is required');
this._page = new Page(opts.session.trim(), opts.idleTimeout, routing.contextId, opts.windowMode, opts.surface, opts.siteSession, routing.preferredContextId);
this._state = 'connected';
Expand All @@ -61,10 +61,11 @@ export class BrowserBridge implements IBrowserFactory {
this._state = 'closed';
}

private async _ensureDaemon(timeoutSeconds?: number, contextId?: string): Promise<void> {
private async _ensureDaemon(timeoutSeconds?: number, contextId?: string, preferredContextId?: string): Promise<void> {
const result = await ensureBrowserBridgeReady({
timeoutSeconds: timeoutSeconds ?? Math.ceil(DAEMON_SPAWN_TIMEOUT / 1000),
contextId,
preferredContextId,
});
this._daemonProc = result.spawnedProcess;
}
Expand Down
25 changes: 23 additions & 2 deletions src/browser/daemon-client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,27 @@ describe('daemon-client', () => {
expect(vi.mocked(fetch).mock.calls[0][0]).toMatch(/\/status\?contextId=work$/);
});

it('fetchDaemonStatus joins contextId and preferredContextId in the status query', async () => {
vi.mocked(fetch).mockResolvedValue({
ok: true,
json: () => Promise.resolve({
ok: true,
pid: 1,
uptime: 0,
extensionConnected: true,
pending: 0,
memoryMB: 1,
port: 19825,
}),
} as Response);

await fetchDaemonStatus({ preferredContextId: 'zvypsyje' });
await fetchDaemonStatus({ contextId: 'work', preferredContextId: 'zvypsyje' });

expect(vi.mocked(fetch).mock.calls[0][0]).toMatch(/\/status\?preferredContextId=zvypsyje$/);
expect(vi.mocked(fetch).mock.calls[1][0]).toMatch(/\/status\?contextId=work&preferredContextId=zvypsyje$/);
});

it('rejects OPENCLI_DAEMON_PORT so CLI and extension cannot split bridge ports', async () => {
vi.resetModules();
vi.stubEnv('OPENCLI_DAEMON_PORT', '19999');
Expand Down Expand Up @@ -444,9 +465,9 @@ describe('daemon-client', () => {
json: () => Promise.resolve({ id: 'server', ok: true, data: 7 }),
} as Response);

await expect(sendCommand('exec', { code: '1 + 6', contextId: 'work' })).resolves.toBe(7);
await expect(sendCommand('exec', { code: '1 + 6', contextId: 'work', preferredContextId: 'zvypsyje' })).resolves.toBe(7);

expect(ensureSpy).toHaveBeenCalledWith(expect.objectContaining({ contextId: 'work', verbose: false }));
expect(ensureSpy).toHaveBeenCalledWith(expect.objectContaining({ contextId: 'work', preferredContextId: 'zvypsyje', verbose: false }));
const ids = fetchMock.mock.calls.map(([, init]) => (JSON.parse(String(init?.body)) as { id: string }).id);
expect(ids).toHaveLength(2);
// Transport retries keep the id stable so the executor's journal can dedupe.
Expand Down
8 changes: 5 additions & 3 deletions src/browser/daemon-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,12 @@ async function sendCommandRaw(
const remainingSeconds = Math.ceil((deadlineAt - Date.now()) / 1000);
const ready = await ensureBrowserBridgeReady({
timeoutSeconds: Math.max(1, Math.min(DEFAULT_BROWSER_CONNECT_TIMEOUT, remainingSeconds)),
// Only an explicit requirement pins readiness to a specific profile —
// waiting for a stale preferred profile to come back would hang the
// ensure path even though the daemon can already serve the command.
// Only an explicit requirement pins readiness to a specific profile; a
// preferred one is arbitrated against live connections on every poll,
// so a stale default still cannot hang the ensure path while a valid
// one avoids the multi-profile ambiguity error (#2259).
contextId,
preferredContextId,
verbose: false,
});
executorJournaled = versionAtLeast(ready.health.status?.extensionVersion, MIN_JOURNAL_EXTENSION_VERSION);
Expand Down
7 changes: 4 additions & 3 deletions src/browser/daemon-lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,15 @@ export async function restartDaemon(opts: { stopTimeoutMs?: number; startTimeout
}

export async function ensureBrowserBridgeReady(
opts: { timeoutSeconds?: number; contextId?: string; verbose?: boolean } = {},
opts: { timeoutSeconds?: number; contextId?: string; preferredContextId?: string; verbose?: boolean } = {},
): Promise<EnsureBrowserBridgeReadyResult> {
const timeoutSeconds = opts.timeoutSeconds && opts.timeoutSeconds > 0 ? opts.timeoutSeconds : 10;
const timeoutMs = timeoutSeconds * 1000;
const verbose = opts.verbose ?? true;
const contextId = opts.contextId;
const preferredContextId = opts.preferredContextId;

const health = await getDaemonHealth({ contextId });
const health = await getDaemonHealth({ contextId, preferredContextId });
const daemonVersion = health.status?.daemonVersion;
const isStale = !!health.status && (!daemonVersion || daemonVersion !== PKG_VERSION);
let staleDaemonReplaced = false;
Expand Down Expand Up @@ -158,7 +159,7 @@ export async function ensureBrowserBridgeReady(
process.stderr.write(' Make sure Chrome or Chromium is open and the OpenCLI extension is enabled.\n');
}

const finalHealth = await waitForBridgeReady(getDaemonHealth, { timeoutMs, contextId });
const finalHealth = await waitForBridgeReady(getDaemonHealth, { timeoutMs, contextId, preferredContextId });
if (finalHealth.state === 'ready') return { health: finalHealth, spawnedProcess };
throw browserConnectErrorFromHealth(finalHealth, contextId);
}
Expand Down
11 changes: 7 additions & 4 deletions src/browser/daemon-transport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,13 @@ export async function requestDaemon(pathname: string, init?: RequestInit & { tim
}
}

export async function fetchDaemonStatus(opts?: { timeout?: number; contextId?: string }): Promise<DaemonStatus | null> {
export async function fetchDaemonStatus(opts?: { timeout?: number; contextId?: string; preferredContextId?: string }): Promise<DaemonStatus | null> {
try {
const params = opts?.contextId ? `?contextId=${encodeURIComponent(opts.contextId)}` : '';
const res = await requestDaemon(`/status${params}`, { timeout: opts?.timeout ?? 2000 });
const query = new URLSearchParams({
...(opts?.contextId ? { contextId: opts.contextId } : {}),
...(opts?.preferredContextId ? { preferredContextId: opts.preferredContextId } : {}),
}).toString();
const res = await requestDaemon(`/status${query ? `?${query}` : ''}`, { timeout: opts?.timeout ?? 2000 });
if (!res.ok) return null;
return await res.json() as DaemonStatus;
} catch (err) {
Expand All @@ -78,7 +81,7 @@ export async function fetchDaemonStatus(opts?: { timeout?: number; contextId?: s
}
}

export async function getDaemonHealth(opts?: { timeout?: number; contextId?: string }): Promise<DaemonHealth> {
export async function getDaemonHealth(opts?: { timeout?: number; contextId?: string; preferredContextId?: string }): Promise<DaemonHealth> {
const status = await fetchDaemonStatus(opts);
if (!status) return { state: 'stopped', status: null };
if (status.profileRequired) return { state: 'profile-required', status };
Expand Down
3 changes: 2 additions & 1 deletion src/daemon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,8 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise
const mem = process.memoryUsage();
const params = new URL(url, `http://localhost:${PORT}`).searchParams;
const requestedContextId = params.get('contextId')?.trim() || undefined;
const route = resolveExtensionConnection(requestedContextId);
const preferredContextId = params.get('preferredContextId')?.trim() || undefined;
const route = resolveExtensionConnection(requestedContextId, preferredContextId);
const profiles = activeProfiles().map((profile) => ({
contextId: profile.contextId,
extensionConnected: true,
Expand Down
26 changes: 26 additions & 0 deletions src/doctor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,32 @@ describe('doctor report rendering', () => {
}
});

it('threads the configured default profile into the health read', async () => {
const fs = await import('node:fs');
const os = await import('node:os');
const path = await import('node:path');
const configDir = fs.mkdtempSync(path.join(os.tmpdir(), 'opencli-doctor-profile-'));
fs.writeFileSync(
path.join(configDir, 'browser-profiles.json'),
JSON.stringify({ version: 1, aliases: {}, defaultContextId: 'zvypsyje' }),
);
vi.stubEnv('OPENCLI_CONFIG_DIR', configDir);
vi.stubEnv('OPENCLI_PROFILE', '');
try {
mockGetDaemonHealth.mockResolvedValueOnce({
state: 'ready',
status: { extensionConnected: true },
});

await runBrowserDoctor();

expect(mockGetDaemonHealth).toHaveBeenCalledWith({ preferredContextId: 'zvypsyje' });
} finally {
vi.unstubAllEnvs();
fs.rmSync(configDir, { recursive: true, force: true });
}
});

it('reports flapping when live check succeeds but final status shows extension disconnected', async () => {
mockGetDaemonHealth.mockResolvedValueOnce({ state: 'no-extension', status: { extensionConnected: false } });

Expand Down
8 changes: 5 additions & 3 deletions src/doctor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { getErrorMessage } from './errors.js';
import { getRuntimeLabel } from './runtime-detect.js';
import { getCachedLatestExtensionVersion } from './update-check.js';
import type { BrowserProfileStatus } from './browser/daemon-transport.js';
import { aliasForContextId, loadProfileConfig } from './browser/profile.js';
import { aliasForContextId, loadProfileConfig, profileRouteParams, resolveProfileSelection } from './browser/profile.js';
import { formatDaemonVersion, isDaemonStale, staleDaemonIssue } from './browser/daemon-version.js';
import { findShadowedUserAdapters, formatAdapterShadowIssue, type AdapterShadow } from './adapter-shadow.js';

Expand Down Expand Up @@ -111,8 +111,10 @@ export async function runBrowserDoctor(opts: DoctorOptions = {}): Promise<Doctor
// (bridge.connect spawns daemon) and validates end-to-end browser bridge health.
const connectivity = await checkConnectivity();

// Single status read *after* connectivity side-effects settle.
const health = await getDaemonHealth();
// Single status read *after* connectivity side-effects settle. Threads the
// profile selection like command dispatch does, so a configured default is
// arbitrated instead of read as multi-profile ambiguity (#2259).
const health = await getDaemonHealth(profileRouteParams(resolveProfileSelection()));
const daemonRunning = health.state !== 'stopped';
const extensionConnected = health.state === 'ready';
const daemonFlaky = connectivity.ok && !daemonRunning;
Expand Down
28 changes: 26 additions & 2 deletions tests/e2e/daemon-transport.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* - the per-command deadline produces a structured 408, not a hang
* - extension disconnect after dispatch yields command_result_unknown
* - a stale preferred profile falls back to the only connected profile
* - /status resolves multi-profile ambiguity through preferredContextId
* - graceful shutdown flushes structured 503s instead of dropping sockets
*
* Requires port 19825 (the fixed bridge port); lives in the e2e-fixed-port
Expand Down Expand Up @@ -96,9 +97,9 @@ class FakeExtension {
}
}

async function getStatus(): Promise<any | null> {
async function getStatus(query = ''): Promise<any | null> {
try {
const res = await fetch(`${BASE}/status`, { headers: HEADERS, signal: AbortSignal.timeout(2_000) });
const res = await fetch(`${BASE}/status${query}`, { headers: HEADERS, signal: AbortSignal.timeout(2_000) });
if (!res.ok) return null;
return await res.json();
} catch {
Expand Down Expand Up @@ -294,6 +295,29 @@ describe('daemon transport contracts (real daemon)', () => {
}
});

it('resolves /status multi-profile ambiguity through preferredContextId', async () => {
if (guard()) return;
const first = new FakeExtension();
const second = new FakeExtension();
await first.connect('ctx-status-a');
await second.connect('ctx-status-b');
try {
// Two live profiles and no hint → ambiguous, the caller must pick.
const ambiguous = await getStatus();
expect(ambiguous?.profileRequired).toBe(true);
expect(ambiguous?.extensionConnected).toBe(false);

// The forwarded preference resolves the ambiguity (#2259).
const preferred = await getStatus('?preferredContextId=ctx-status-b');
expect(preferred?.profileRequired).toBe(false);
expect(preferred?.contextId).toBe('ctx-status-b');
expect(preferred?.extensionConnected).toBe(true);
} finally {
first.close();
second.close();
}
});

it('flushes a structured daemon_shutting_down 503 to in-flight dispatched commands on shutdown', async () => {
if (guard()) return;
const ext = new FakeExtension();
Expand Down