Skip to content
Merged
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
8 changes: 5 additions & 3 deletions src/globalConfig/accessor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,10 @@ export class DefaultGlobalConfigAccessor implements GlobalConfigAccessor {

const configFileData = await this.readConfigFile();

// if no installationId is present, generate one and merge it into the file data
if (!configFileData.installationId) {

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.

Would it be simpler to return isFirstRun in the output here? Would that avoid having to make many of these other changes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this approach, (and actually implemented it initially), but I didn't like the behavior of get writing to the config as a side effect since a caller would expect a strict-read here.

However, I can see the argument that from the perspective of the caller, it is a strict-read still.

Let me swap to the simpler approach and if we see issues with the side-effect, we can revisit.

// a run with no persisted installationId is the first run on this machine
const isFirstRun = !configFileData.installationId;

if (isFirstRun) {
configFileData.installationId = DEFAULT_GLOBAL_CONFIG.installationId;
this.logger.info(`no installationId found, persisting one`);

Expand All @@ -61,7 +63,7 @@ export class DefaultGlobalConfigAccessor implements GlobalConfigAccessor {
}
}

this.cachedConfig = applyOverrides(DEFAULT_GLOBAL_CONFIG, configFileData);
this.cachedConfig = { ...applyOverrides(DEFAULT_GLOBAL_CONFIG, configFileData), isFirstRun };
return this.cachedConfig;
}

Expand Down
2 changes: 1 addition & 1 deletion src/globalConfig/types.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const globalConfigFileSchema = z.object({
export type GlobalConfigFileData = z.infer<typeof globalConfigFileSchema>;

/** The fully resolved config after applying defaults — all fields required. */
export type GlobalConfig = DeepRequired<GlobalConfigFileData>;
export type GlobalConfig = DeepRequired<GlobalConfigFileData> & { isFirstRun?: boolean };

/** Manages access to a set of configuration values for the CLI */
export interface GlobalConfigAccessor {
Expand Down
5 changes: 3 additions & 2 deletions src/handlers/config/handler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ export const createConfigHandler = () =>
const jsonRenderer = ctx.require(JsonRendererKey);

const globalConfig = await globalConfigAccessor.get();
// print entire config when key is missing.
// isFirstRun is not user controlled, so strip from the output.
if (!args.key) {
jsonRenderer.renderJson(globalConfig);
const { isFirstRun: _isFirstRun, ...persistedConfig } = globalConfig;
jsonRenderer.renderJson(persistedConfig);
return;
}

Expand Down
10 changes: 9 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { FsReadWriteJson } from "./io";
import { createFileLogger, LOG_LEVEL } from "./logging";
import { runWithExitCode } from "./runnable";
import { DefaultGlobalConfigAccessor } from "./globalConfig";
import { DefaultTelemetryClient } from "./telemetry";
import { DefaultTelemetryClient, printFirstRunNotice } from "./telemetry";
import { AgentCoreCLIError } from "./errors";
import { PACKAGE_VERSION } from "./constants";
import { CommandRunMetricEventKey, ValueContext } from "./router";
Expand Down Expand Up @@ -61,6 +61,8 @@ process.exit(
exit_reason: "success",
});

const globalConfig = await globalConfigAccessor.get();

try {
rootLogger.info(`running CLI`);

Expand Down Expand Up @@ -112,6 +114,12 @@ process.exit(
}
await telemetryClient.shutdown();
await rootLogger.end();

printFirstRunNotice(
globalConfig.isFirstRun ?? false,
globalConfig.telemetry.enabled,
io.stderr,
);
}
}),
);
1 change: 1 addition & 0 deletions src/telemetry/index.tsx
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export { DefaultTelemetryClient } from "./client";
export { printFirstRunNotice } from "./notice";
export { type AttributesOf, type MetricEvent } from "./types";
26 changes: 26 additions & 0 deletions src/telemetry/notice.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { test, describe, expect } from "bun:test";
import { printFirstRunNotice } from "./notice";

describe("printFirstRunNotice", () => {
test.each([
[true, true, 1],
[true, false, 0],
[false, true, 0],
[false, false, 0],
])(
"isFirstRun=%p telemetryEnabled=%p writes the notice %p time(s)",
(isFirstRun, telemetryEnabled, expectedWrites) => {
const written: string[] = [];

printFirstRunNotice(isFirstRun, telemetryEnabled, {
write: (text) => void written.push(text),
});

expect(written).toHaveLength(expectedWrites);
if (expectedWrites > 0) {
expect(written[0]).toContain("collects aggregated, anonymous usage analytics");
expect(written[0]).toContain("agentcore config telemetry.enabled false");
}
},
);
});
21 changes: 21 additions & 0 deletions src/telemetry/notice.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* Writes the telemetry-collection notice to the given stream on the first run of
* the CLI, unless telemetry is already disabled.
*/
export function printFirstRunNotice(

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.

Can we shift this function under the handler file since it's only used by the handler?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to keep the root level entrypoint clean, and splitting it out made it easier/more natural to add unit tests, but I agree exporting this from telemetry module for a single consumer adds some indirection that shouldn't be necessary.

isFirstRun: boolean,
telemetryEnabled: boolean,
out: { write(text: string): void },
): void {
if (!isFirstRun || !telemetryEnabled) return;

out.write(
[
"",
"The AgentCore CLI collects aggregated, anonymous usage analytics to help improve the tool.",
"To opt out: agentcore config telemetry.enabled false",
"To audit: agentcore config telemetry.audit true",
"",
].join("\n"),
);
}
Loading