Skip to content

Commit 42fbf85

Browse files
amankansal-ltclaude
amankansal-lt
and
claude
committed
refactor(provider-webdriver): build the malformed bs:// rejection in one place
Connect, connection verification and the runtime resolver each built "BrowserStack --provider-app <app> is not a bs:// app id." with different details, so a hand-authored remote config got no hint. parseBrowserStackAppReference now owns the grammar check and the INVALID_ARGS it throws, with the same providerApp and hint on every path. resolveHubAppReference takes that parser instead of a canonicalizer and a predicate. It lives in browserstack.ts rather than providers.ts because the providers subpath evaluates no other module, and every caller already loads browserstack.ts. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent 4e635cc commit 42fbf85

9 files changed

Lines changed: 66 additions & 53 deletions

‎packages/provider-webdriver/src/browserstack-connection-verification.ts‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import path from 'node:path';
22
import { AppError } from '@agent-device/kernel/errors';
3-
import { canonicalBrowserStackAppReference, isBrowserStackAppReference } from './providers.ts';
3+
import { parseBrowserStackAppReference } from './browserstack.ts';
4+
import { isBrowserStackAppReference } from './providers.ts';
45
import { fetchProviderVerificationJson, sameOsVersion } from './webdriver-utils.ts';
56
import type {
67
CloudWebDriverConnectionVerification,
@@ -65,12 +66,7 @@ export async function verifyBrowserStackConnection(
6566

6667
/** Hand-authored remote configs reach verification without passing through connect's normalization. */
6768
function readBrowserStackAppOption(app: string): string {
68-
const reference = canonicalBrowserStackAppReference(app);
69-
if (reference === undefined) return app;
70-
if (isBrowserStackAppReference(reference)) return reference;
71-
throw new AppError('INVALID_ARGS', `BrowserStack --provider-app ${app} is not a bs:// app id.`, {
72-
providerApp: app,
73-
});
69+
return parseBrowserStackAppReference(app) ?? app;
7470
}
7571

7672
async function verifyBrowserStackApp(

‎packages/provider-webdriver/src/browserstack.test.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -215,13 +215,16 @@ test('BrowserStack canonicalizes the bs:// scheme and refuses an id outside its
215215
assert.equal(await resolve('BS://app-id'), 'bs://app-id');
216216
assert.equal(await resolve('HTTPS://builds.example/App.apk'), 'HTTPS://builds.example/App.apk');
217217
for (const app of ['bs://', 'bs://a b', 'Bs://a/b']) {
218-
await assert.rejects(
219-
resolve(app),
220-
(error: unknown) =>
221-
error instanceof AppError &&
222-
error.code === 'INVALID_ARGS' &&
223-
error.message === `BrowserStack --provider-app ${app} is not a bs:// app id.`,
224-
);
218+
await assert.rejects(resolve(app), (error: unknown) => {
219+
assert.ok(error instanceof AppError);
220+
assert.equal(error.code, 'INVALID_ARGS');
221+
assert.equal(error.message, `BrowserStack --provider-app ${app} is not a bs:// app id.`);
222+
assert.deepEqual(error.details, {
223+
providerApp: app,
224+
hint: 'Pass <bs://app-id-or-local-path>.',
225+
});
226+
return true;
227+
});
225228
}
226229
await assert.rejects(
227230
resolve('App.app'),

‎packages/provider-webdriver/src/browserstack.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { CloudArtifact, CloudArtifactsResult } from '@agent-device/contracts/observability';
2+
import { AppError } from '@agent-device/kernel/errors';
23
import type { CloudWebDriverCapabilityOverrides } from './capabilities.ts';
34
import type { CloudWebDriverUploadApp } from './runtime.ts';
45
import { cloudArtifactsReadyOrPending, urlArtifactFromDetails } from './artifact-results.ts';
@@ -108,6 +109,19 @@ export function createBrowserStackUploadApp(
108109
);
109110
}
110111

112+
/**
113+
* The canonical `bs://` reference for `app`, or undefined when `app` does not use the scheme. A
114+
* `bs://` value outside the id grammar is `INVALID_ARGS`, worded the same on every path.
115+
*/
116+
export function parseBrowserStackAppReference(app: string): string | undefined {
117+
const reference = canonicalBrowserStackAppReference(app);
118+
if (reference === undefined || isBrowserStackAppReference(reference)) return reference;
119+
throw new AppError('INVALID_ARGS', `BrowserStack --provider-app ${app} is not a bs:// app id.`, {
120+
providerApp: app,
121+
hint: 'Pass <bs://app-id-or-local-path>.',
122+
});
123+
}
124+
111125
/** The hub fetches a public URL itself, so only a local path is uploaded. */
112126
export async function resolveBrowserStackAppReference(
113127
app: string,
@@ -118,8 +132,7 @@ export async function resolveBrowserStackAppReference(
118132
app,
119133
cwd: options.cwd,
120134
referenceLabel: 'a bs:// app id',
121-
canonicalReference: canonicalBrowserStackAppReference,
122-
isReference: isBrowserStackAppReference,
135+
parseReference: parseBrowserStackAppReference,
123136
uploadFile: async (appPath, signal) => await uploadBrowserStackApp(appPath, options, signal),
124137
signal: options.signal,
125138
});

‎packages/provider-webdriver/src/connection-verification.test.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,16 @@ test('BrowserStack verification canonicalizes the bs:// scheme and refuses an id
147147
fetchMock.mockClear();
148148
await assert.rejects(
149149
createProvider().verifyConnection({ ...browserStackOptions, app: 'bs://a b' }),
150-
(error: unknown) =>
151-
error instanceof AppError &&
152-
error.code === 'INVALID_ARGS' &&
153-
error.message === 'BrowserStack --provider-app bs://a b is not a bs:// app id.',
150+
(error: unknown) => {
151+
assert.ok(error instanceof AppError);
152+
assert.equal(error.code, 'INVALID_ARGS');
153+
assert.equal(error.message, 'BrowserStack --provider-app bs://a b is not a bs:// app id.');
154+
assert.deepEqual(error.details, {
155+
providerApp: 'bs://a b',
156+
hint: 'Pass <bs://app-id-or-local-path>.',
157+
});
158+
return true;
159+
},
154160
);
155161
assert.equal(fetchMock.mock.calls.length, 0);
156162
});

‎packages/provider-webdriver/src/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import type { CloudWebDriverRuntime } from './runtime.ts';
1919

2020
export { CLOUD_WEBDRIVER_PROVIDERS };
2121
export { readAwsDeviceFarmRegionFromArn };
22+
export { parseBrowserStackAppReference } from './browserstack.ts';
2223
export { rejectBrowserStackOnlyDeviceFeatures } from './browserstack-device-features.ts';
2324
export type { CloudWebDriverKnownProviderName } from './providers.ts';
2425
export type { ProviderWebDriverDependencies, RunHostCommand } from './dependencies.ts';

‎packages/provider-webdriver/src/webdriver-utils.test.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,12 @@ test('the hub install adapter uploads the build and launches the hinted app', as
9292
});
9393

9494
const hubReferenceGrammar = {
95-
canonicalReference: (app: string) =>
96-
app.slice(0, 6).toLowerCase() === 'hub://' ? `hub://${app.slice(6)}` : undefined,
97-
isReference: (reference: string) => /^hub:\/\/\w+$/.test(reference),
95+
parseReference: (app: string) => {
96+
if (app.slice(0, 6).toLowerCase() !== 'hub://') return undefined;
97+
const reference = `hub://${app.slice(6)}`;
98+
if (/^hub:\/\/\w+$/.test(reference)) return reference;
99+
throw new AppError('INVALID_ARGS', `Hub --provider-app ${app} is not a hub:// app id.`);
100+
},
98101
};
99102

100103
test('the hub app resolver passes references through, uploads local files, and passes URLs through', async () => {
@@ -171,7 +174,7 @@ test('appending a route keeps a query on the base endpoint', () => {
171174
);
172175
});
173176

174-
test('the hub app resolver refuses a malformed reference, typed', async () => {
177+
test('the hub app resolver surfaces the grammar rejection of a malformed reference without uploading', async () => {
175178
const tempDir = await mkdtempForTest('agent-device-hub-resolve-invalid-');
176179
try {
177180
const uploadFile = vi.fn(async () => 'hub://never');

‎packages/provider-webdriver/src/webdriver-utils.ts‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -161,22 +161,17 @@ export async function resolveHubAppReference(options: {
161161
cwd?: string;
162162
/** How the scheme reads in the error message, e.g. `a bs:// app id`. */
163163
referenceLabel: string;
164-
/** The canonical spelling of the hub's own reference, or undefined when `app` is not one. */
165-
canonicalReference: (app: string) => string | undefined;
166-
isReference: (reference: string) => boolean;
164+
/**
165+
* The canonical spelling of the hub's own reference, or undefined when `app` is not one. Throws
166+
* when `app` uses the hub's scheme outside its grammar.
167+
*/
168+
parseReference: (app: string) => string | undefined;
167169
uploadFile: (appPath: string, signal?: AbortSignal) => Promise<string>;
168170
signal?: AbortSignal;
169171
}): Promise<string> {
170172
const { app } = options;
171-
const reference = options.canonicalReference(app);
172-
if (reference !== undefined) {
173-
if (options.isReference(reference)) return reference;
174-
throw new AppError(
175-
'INVALID_ARGS',
176-
`${options.service} --provider-app ${app} is not ${options.referenceLabel}.`,
177-
{ providerApp: app },
178-
);
179-
}
173+
const reference = options.parseReference(app);
174+
if (reference !== undefined) return reference;
180175
if (/^https?:\/\//i.test(app)) return app;
181176
const appPath = path.resolve(options.cwd ?? process.cwd(), app);
182177
if (!fs.existsSync(appPath)) {

‎src/__tests__/cloud-connect-profile.test.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -451,10 +451,16 @@ test('connect browserstack canonicalizes the app scheme and refuses a malformed
451451
env: { BROWSERSTACK_USERNAME: 'browser-user', BROWSERSTACK_ACCESS_KEY: 'browser-key' },
452452
flags: { json: false, help: false, version: false, ...flags, providerApp: app },
453453
}),
454-
(error: unknown) =>
455-
error instanceof AppError &&
456-
error.code === 'INVALID_ARGS' &&
457-
error.message === `BrowserStack --provider-app ${app} is not a bs:// app id.`,
454+
(error: unknown) => {
455+
assert.ok(error instanceof AppError);
456+
assert.equal(error.code, 'INVALID_ARGS');
457+
assert.equal(error.message, `BrowserStack --provider-app ${app} is not a bs:// app id.`);
458+
assert.deepEqual(error.details, {
459+
providerApp: app,
460+
hint: 'Pass <bs://app-id-or-local-path>.',
461+
});
462+
return true;
463+
},
458464
);
459465
}
460466
} finally {

‎src/cli/connection/cloud-webdriver-profile.ts‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,10 @@
11
import {
22
CLOUD_WEBDRIVER_PROVIDERS,
3+
parseBrowserStackAppReference,
34
readAwsDeviceFarmRegionFromArn,
45
rejectBrowserStackOnlyDeviceFeatures,
56
type CloudWebDriverKnownProviderName,
67
} from '@agent-device/provider-webdriver';
7-
import {
8-
canonicalBrowserStackAppReference,
9-
isBrowserStackAppReference,
10-
} from '@agent-device/provider-webdriver/providers';
118
import type { RemoteConfigProfile } from '../../remote/remote-config-schema.ts';
129
import { AppError } from '@agent-device/kernel/errors';
1310
import type { PlatformSelector } from '@agent-device/kernel/device';
@@ -131,15 +128,8 @@ function browserStackProfileFields(options: {
131128

132129
function normalizeBrowserStackAppReference(app: string, cwd: string): string {
133130
if (/^https?:\/\//i.test(app)) return app;
134-
const reference = canonicalBrowserStackAppReference(app);
135-
if (reference !== undefined) {
136-
if (isBrowserStackAppReference(reference)) return reference;
137-
throw new AppError(
138-
'INVALID_ARGS',
139-
`BrowserStack --provider-app ${app} is not a bs:// app id.`,
140-
{ hint: 'Pass <bs://app-id-or-local-path>.' },
141-
);
142-
}
131+
const reference = parseBrowserStackAppReference(app);
132+
if (reference !== undefined) return reference;
143133
const resolvedPath = path.resolve(cwd, app);
144134
try {
145135
if (fs.statSync(resolvedPath).isFile()) return resolvedPath;

0 commit comments

Comments
 (0)