Skip to content

Commit 348efa9

Browse files
HweinstockHweinstock
committed
fix: address review feedback — guard telemetry init, replaceAll, unknown fallback, TUI try/catch
1 parent bc3a756 commit 348efa9

9 files changed

Lines changed: 150 additions & 108 deletions

‎src/cli/primitives/CredentialPrimitive.tsx‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -342,24 +342,29 @@ export class CredentialPrimitive extends BasePrimitive<AddCredentialOptions, Rem
342342
};
343343
});
344344
} else {
345-
// TUI fallback — dynamic imports to avoid pulling ink (async) into registry
346-
requireTTY();
347-
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
348-
import('ink'),
349-
import('react'),
350-
import('../tui/screens/add/AddFlow'),
351-
]);
352-
const { clear, unmount } = render(
353-
React.createElement(AddFlow, {
354-
isInteractive: false,
355-
initialResource: 'credential',
356-
onExit: () => {
357-
clear();
358-
unmount();
359-
process.exit(0);
360-
},
361-
})
362-
);
345+
try {
346+
// TUI fallback — dynamic imports to avoid pulling ink (async) into registry
347+
requireTTY();
348+
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
349+
import('ink'),
350+
import('react'),
351+
import('../tui/screens/add/AddFlow'),
352+
]);
353+
const { clear, unmount } = render(
354+
React.createElement(AddFlow, {
355+
isInteractive: false,
356+
initialResource: 'credential',
357+
onExit: () => {
358+
clear();
359+
unmount();
360+
process.exit(0);
361+
},
362+
})
363+
);
364+
} catch (error) {
365+
console.error(getErrorMessage(error));
366+
process.exit(1);
367+
}
363368
}
364369
}
365370
);

‎src/cli/primitives/EvaluatorPrimitive.ts‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -320,24 +320,29 @@ export class EvaluatorPrimitive extends BasePrimitive<AddEvaluatorOptions, Remov
320320
};
321321
});
322322
} else {
323-
// TUI fallback
324-
requireTTY();
325-
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
326-
import('ink'),
327-
import('react'),
328-
import('../tui/screens/add/AddFlow'),
329-
]);
330-
const { clear, unmount } = render(
331-
React.createElement(AddFlow, {
332-
isInteractive: false,
333-
initialResource: 'evaluator',
334-
onExit: () => {
335-
clear();
336-
unmount();
337-
process.exit(0);
338-
},
339-
})
340-
);
323+
try {
324+
// TUI fallback
325+
requireTTY();
326+
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
327+
import('ink'),
328+
import('react'),
329+
import('../tui/screens/add/AddFlow'),
330+
]);
331+
const { clear, unmount } = render(
332+
React.createElement(AddFlow, {
333+
isInteractive: false,
334+
initialResource: 'evaluator',
335+
onExit: () => {
336+
clear();
337+
unmount();
338+
process.exit(0);
339+
},
340+
})
341+
);
342+
} catch (error) {
343+
console.error(getErrorMessage(error));
344+
process.exit(1);
345+
}
341346
}
342347
}
343348
);

‎src/cli/primitives/GatewayTargetPrimitive.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -330,10 +330,10 @@ export class GatewayTargetPrimitive extends BasePrimitive<AddGatewayTargetOption
330330

331331
const cliType = cliOptions.type ?? '';
332332
const telemetryTargetType =
333-
cliType in targetTypeMap ? targetTypeMap[cliType as TargetTypeKey] : ('mcp-server' as const);
333+
cliType in targetTypeMap ? targetTypeMap[cliType as TargetTypeKey] : ('unknown' as const);
334334
const telemetryOutboundAuth = standardize(
335335
OutboundAuth,
336-
(cliOptions.outboundAuthType ?? 'none').replace('_', '-')
336+
(cliOptions.outboundAuthType ?? 'none').replaceAll('_', '-')
337337
);
338338
const telemetryHost = standardize(GatewayTargetHost, cliOptions.host ?? 'lambda');
339339

‎src/cli/primitives/MemoryPrimitive.tsx‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -240,24 +240,29 @@ export class MemoryPrimitive extends BasePrimitive<AddMemoryOptions, RemovableMe
240240
};
241241
});
242242
} else {
243-
// TUI fallback — dynamic imports to avoid pulling ink (async) into registry
244-
requireTTY();
245-
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
246-
import('ink'),
247-
import('react'),
248-
import('../tui/screens/add/AddFlow'),
249-
]);
250-
const { clear, unmount } = render(
251-
React.createElement(AddFlow, {
252-
isInteractive: false,
253-
initialResource: 'memory',
254-
onExit: () => {
255-
clear();
256-
unmount();
257-
process.exit(0);
258-
},
259-
})
260-
);
243+
try {
244+
// TUI fallback — dynamic imports to avoid pulling ink (async) into registry
245+
requireTTY();
246+
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
247+
import('ink'),
248+
import('react'),
249+
import('../tui/screens/add/AddFlow'),
250+
]);
251+
const { clear, unmount } = render(
252+
React.createElement(AddFlow, {
253+
isInteractive: false,
254+
initialResource: 'memory',
255+
onExit: () => {
256+
clear();
257+
unmount();
258+
process.exit(0);
259+
},
260+
})
261+
);
262+
} catch (error) {
263+
console.error(getErrorMessage(error));
264+
process.exit(1);
265+
}
261266
}
262267
}
263268
);

‎src/cli/primitives/OnlineEvalConfigPrimitive.ts‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -170,24 +170,29 @@ export class OnlineEvalConfigPrimitive extends BasePrimitive<AddOnlineEvalConfig
170170
};
171171
});
172172
} else {
173-
// TUI fallback
174-
requireTTY();
175-
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
176-
import('ink'),
177-
import('react'),
178-
import('../tui/screens/add/AddFlow'),
179-
]);
180-
const { clear, unmount } = render(
181-
React.createElement(AddFlow, {
182-
isInteractive: false,
183-
initialResource: 'online-eval',
184-
onExit: () => {
185-
clear();
186-
unmount();
187-
process.exit(0);
188-
},
189-
})
190-
);
173+
try {
174+
// TUI fallback
175+
requireTTY();
176+
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
177+
import('ink'),
178+
import('react'),
179+
import('../tui/screens/add/AddFlow'),
180+
]);
181+
const { clear, unmount } = render(
182+
React.createElement(AddFlow, {
183+
isInteractive: false,
184+
initialResource: 'online-eval',
185+
onExit: () => {
186+
clear();
187+
unmount();
188+
process.exit(0);
189+
},
190+
})
191+
);
192+
} catch (error) {
193+
console.error(getErrorMessage(error));
194+
process.exit(1);
195+
}
191196
}
192197
}
193198
);

‎src/cli/primitives/PolicyEnginePrimitive.ts‎

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -272,22 +272,27 @@ export class PolicyEnginePrimitive extends BasePrimitive<AddPolicyEngineOptions,
272272
};
273273
});
274274
} else {
275-
requireTTY();
276-
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
277-
import('ink'),
278-
import('react'),
279-
import('../tui/screens/add/AddFlow'),
280-
]);
281-
const { clear, unmount } = render(
282-
React.createElement(AddFlow, {
283-
isInteractive: false,
284-
onExit: () => {
285-
clear();
286-
unmount();
287-
process.exit(0);
288-
},
289-
})
290-
);
275+
try {
276+
requireTTY();
277+
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
278+
import('ink'),
279+
import('react'),
280+
import('../tui/screens/add/AddFlow'),
281+
]);
282+
const { clear, unmount } = render(
283+
React.createElement(AddFlow, {
284+
isInteractive: false,
285+
onExit: () => {
286+
clear();
287+
unmount();
288+
process.exit(0);
289+
},
290+
})
291+
);
292+
} catch (error) {
293+
console.error(getErrorMessage(error));
294+
process.exit(1);
295+
}
291296
}
292297
}
293298
);

‎src/cli/primitives/PolicyPrimitive.ts‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -348,23 +348,28 @@ export class PolicyPrimitive extends BasePrimitive<AddPolicyOptions, RemovablePo
348348
};
349349
});
350350
} else {
351-
requireTTY();
352-
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
353-
import('ink'),
354-
import('react'),
355-
import('../tui/screens/add/AddFlow'),
356-
]);
357-
const { clear, unmount } = render(
358-
React.createElement(AddFlow, {
359-
isInteractive: false,
360-
initialResource: 'policy',
361-
onExit: () => {
362-
clear();
363-
unmount();
364-
process.exit(0);
365-
},
366-
})
367-
);
351+
try {
352+
requireTTY();
353+
const [{ render }, { default: React }, { AddFlow }] = await Promise.all([
354+
import('ink'),
355+
import('react'),
356+
import('../tui/screens/add/AddFlow'),
357+
]);
358+
const { clear, unmount } = render(
359+
React.createElement(AddFlow, {
360+
isInteractive: false,
361+
initialResource: 'policy',
362+
onExit: () => {
363+
clear();
364+
unmount();
365+
process.exit(0);
366+
},
367+
})
368+
);
369+
} catch (error) {
370+
console.error(getErrorMessage(error));
371+
process.exit(1);
372+
}
368373
}
369374
}
370375
);

‎src/cli/telemetry/cli-command-run.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,25 @@ import type { Command, CommandAttrs } from './schemas/command-run.js';
55
/**
66
* Run a CLI command with telemetry, standardized error output, and process.exit.
77
* The callback should throw on failure and return telemetry attrs on success.
8+
*
9+
* If telemetry initialization fails, the command still runs without telemetry —
10+
* telemetry must never block CLI behavior.
811
*/
912
export async function cliCommandRun<C extends Command>(
1013
command: C,
1114
json: boolean,
1215
fn: () => Promise<CommandAttrs<C>>
1316
): Promise<never> {
1417
try {
15-
const client = await TelemetryClientAccessor.get();
18+
let client;
19+
try {
20+
client = await TelemetryClientAccessor.get();
21+
} catch {
22+
// Telemetry init failed — run without it
23+
await fn();
24+
process.exit(0);
25+
}
26+
// withCommandRun records success/failure telemetry, then re-throws on failure
1627
await client.withCommandRun(command, fn);
1728
process.exit(0);
1829
} catch (error) {

‎src/cli/telemetry/schemas/common-shapes.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ export const GatewayTargetType = z.enum([
4747
'open-api-schema',
4848
'smithy-model',
4949
'lambda-function-arn',
50+
'unknown',
5051
]);
5152
export const Language = z.enum(['python', 'typescript', 'other']);
5253
export const Level = z.enum(['session', 'trace', 'tool_call']);

0 commit comments

Comments
 (0)