feat(config): implement global config to wire up config command

[Hweinstock]

Problem

The CLI is missing a way to view and control global configuration. There is an existing config command, but its not wired up.

Solution

• implement a new top level globalConfig/ module that defines an accessor for global config. The global config is decoupled from the source of the data via an injected dependency.
• move src/router/schemas.tsx to top level /parsing directory so that logic can be re-used.
• global config is type-safe based on the global config schema using json dot notation. For example, valid keys are telemetry, telemetry.enabled, etc. All of which are derived automatically from the schema definition.
• wire up config command to be output JSON responses.

Testing

• wrote unit tests at the config command level to test e2e, rather than testing implementation directly.
• e2e testing using existing config from CLI also works.
  Ex.

> bun run start config
{
  "telemetry": {
    "enabled": false,
    "endpoint": "https://telemetry.agentcore.aws.dev"
  },
}
> bun run start config telemetry.endpoint 
"https://telemetry.agentcore.aws.dev"

> bun run start config telemetry.endpoint https://example.com
"https://example.com"

Future Work

• add error types for telemetry, and easier reasoning (few todo comments on this).
• add logic to setup installationId for telemetry (getOrPut).

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.27559% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.85%. Comparing base (272e50e) to head (b298004).

Files with missing lines	Patch %	Lines
src/handlers/config/handler.tsx	85.36%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           refactor    #1789    +/-   ##
==========================================
  Coverage     94.84%   94.85%            
==========================================
  Files           143      149     +6     
  Lines          7120     7367   +247     
==========================================
+ Hits           6753     6988   +235     
- Misses          367      379    +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Hweinstock]

typecheck fixed in #1756 (comment)

[Hweinstock]

rebased onto create command work.

[AlexanderRichey]

I feel that the level of complexity this PR introduces is out of proportion with the functionality gained. Let's sync on this and see if we can develop a simpler approach.

[AlexanderRichey]

I have some prior art here that might be useful to look at: https://github.com/AlexanderRichey/yagss/blob/main/internal/builder/config.go

[Hweinstock]

converted to draft while I rework based on offline discussion from above, will take out of draft when ready for another review.

[Hweinstock]

Reworked the implementation to simplify:

• config access is now done directly on a typed object rather than a type safe datastore interface.
• partially invalid configs are no longer supported.
• mid-execution config updates are no longer respected.
• no longer leverages shared coercion functions from router, instead leverages zod preprocessing.

[Hweinstock]

taking back into draft to address some comments on #1794, that are relevant here as well. Specifically,

• use type instead of interface for non-extended shape.
• prefer classes over direct definitions.

[AlexanderRichey]

This is a nice improvement over the initial rev, but I still think there's a lot of room to simplify. Let me sketch what I have in mind.

First, ReadWriteJson needs to become generic to justify itself as an interface. The current JsonDataSource is way too specific—and it's name is a bit confusing since it can only be used to read and write one file. Here's an implementation, thanks to Claude, that should handle everything we need to.

import { readFileSync, writeFileSync } from "node:fs";
import { z } from "zod";

class DeserializationError extends Error {
  constructor(path: string, options?: { cause?: unknown }) {
    super(`Failed to deserialize JSON at "${path}"`, options);
    this.name = "DeserializationError";
  }
}

interface ReadWriteJson {
  read<T>(path: string, schema: z.ZodType<T>): T;
  write<T>(path: string, data: T): void;
}

class JsonFileStore implements ReadWriteJson {
  read<T>(path: string, schema: z.ZodType<T>): T {
    const contents = readFileSync(path, "utf-8");

    let parsed: unknown;
    try {
      parsed = JSON.parse(contents);
    } catch (cause) {
      // The file wasn't valid JSON.
      throw new DeserializationError(path, { cause });
    }

    const result = schema.safeParse(parsed);
    if (!result.success) {
      // The file was valid JSON but didn't match the shape of T.
      throw new DeserializationError(path, { cause: result.error });
    }
    return result.data;
  }

  write<T>(path: string, data: T): void {
    const contents = JSON.stringify(data, null, 2);
    writeFileSync(path, contents, "utf-8");
  }
}

With this, the implementation of GlobalConfigAccessor should become straightforward.

// ---------------------------------------------------------------------------
// Global config schema and types
// ---------------------------------------------------------------------------

export const globalConfigSchema = z.object({
  telemetry: z
    .object({
      enabled: z.boolean().optional(),
      endpoint: z.string().optional(),
      audit: z.boolean().optional(),
    })
    .optional(),
  installationId: z.uuid().optional(),
});

/** Inferred shape of the global config from {@link globalConfigSchema}. */
export type GlobalConfig = z.infer<typeof globalConfigSchema>;

/** Reads and writes global CLI configuration. */
export interface GlobalConfigAccessor {
  /** Returns the current global config. */
  get(): Promise<GlobalConfig>;
  /** Validates and persists a new config. Throws on invalid shape. */
  set(updated: GlobalConfig): Promise<GlobalConfig>;
}

// ---------------------------------------------------------------------------
// Accessor implementation
// ---------------------------------------------------------------------------

export class DefaultGlobalConfigAccessor implements GlobalConfigAccessor {
  constructor(
    private readonly store: ReadWriteJson,
    private readonly path: string,
  ) {}

  async get(): Promise<GlobalConfig> {
    try {
      return this.store.read(this.path, globalConfigSchema);
    } catch (err) {
      if (isFileNotFound(err)) {
        // No config yet (first run, fresh install). An empty object is a
        // valid GlobalConfig because every field in the schema is optional.
        // Or throw some error here.
        return {};
      }
      throw err;
    }
  }

  async set(updated: GlobalConfig): Promise<GlobalConfig> {
    // Even though `updated` is typed as GlobalConfig, we re-validate at
    // runtime: the type is only a compile-time guarantee, and callers can
    // defeat it (e.g. a value cast with `as`, or data that originated as
    // `unknown`). parse() throws a ZodError on invalid shape, satisfying
    // the interface's "throws on invalid shape" contract.
    const validated = globalConfigSchema.parse(updated);
    this.store.write(this.path, validated);
    return validated;
  }
}

function isFileNotFound(err: unknown): boolean {
  return (
    typeof err === "object" &&
    err !== null &&
    "code" in err &&
    (err as { code?: unknown }).code === "ENOENT"
  );
}

Now, all you'd have to do is wire this stuff up in the handler chain using DI.

const store = new JsonFileStore();
const accessor = new DefaultGlobalConfigAccessor(store, "/path/to/config.json");

Note: We might want to adjust ReadWriteJson so that it's async, which would be more consistent with the whole codebase.

[Hweinstock]

Aligned implementation closer with proposal. swapped to async, changed some names, and swapped the set argument to be untyped to avoid having to lie to the typesystem to pass the parameter.

[AlexanderRichey]

This is looking really good. The main things blocking for me at the moment are (1) supporting (what I think are unnecessary) bool types, when JSON has a native bool type, and (2) adding a whole bunch of code to support isValidKey() when simpler options seem to be available.

[AlexanderRichey]

Why not use the GlobalConfig type here? That would give us type-safety and any non-GlobalConfig value passed would fail at compile time.

[Hweinstock]

answered below, but I'm aligned with typing it here.

[AlexanderRichey]

This should probably be in its own json package. We can handle that in a follow up.

[Hweinstock]

+1, I figure we pull it out once we have another consumer.

[AlexanderRichey]

Is this the current behavior? If not, I don't think we should support this. JSON has bools.

[Hweinstock]

discussed in person, this was for coercion, but has a bad side effect of accepting odd looking configs.

[AlexanderRichey]

I'm not sure I'm following why we need this. We read a JSON string with JSON.parse, which should parse nested objects just fine, if that's what this is intended to handle.

[AlexanderRichey]

What's the argument in favor of using coerceObject and coercedBoolean over standard object and bool?

[Hweinstock]

removed based on offline conversation

[AlexanderRichey]

I'm not following what all this code is for.

[Hweinstock]

answered offline.

[AlexanderRichey]

Same as above comment. This should accept a type.

[Hweinstock]

I was originally thinking its more "honest" since we already validate it internally to accept unknown. This also avoids the cast at the config handler call site. However, I think I agree that its easier to type this, and cast for exceptions so that we get the static check in as many places as possible.

[AlexanderRichey]

Shouldn't the options be enumerated here? Enumerating the options here would be far simpler than the current isValidKey() code.

[Hweinstock]

I like that idea!

[Hweinstock]

Taking into draft while I address the comments and look to simplify.

[Hweinstock]

one other approach I considered is duplicating the schema into:

const CONFIG_KEY_TYPES = {
    "telemetry": "object",
    "telemetry.enabled": "boolean",
    "telemetry.endpoint": "string",
    "telemetry.audit": "boolean",
    "installationId": "string",
  };

Then using that instead of walking the zod schema to validate keys and paths. It would remove some complexity at the cost of maintaining two configs.

However, I feel like building this at runtime is easier and avoids the drift (but does introduce more complexity). Especially since if we go with the explicit duplicate mapping, we'd probably want a unit test verifying they don't drift which might result in writing the same code anyway.

[Hweinstock]

Main changes:

• now treats the file as a source of overrides, rather than source of truth.
• maintains a default global config in memory and a resolve function to allow file values to override (env vars too at some point).
• all coercion happens in the handler by walking the default config value.

[AlexanderRichey]

Any reason why we can't have this?

const DEFAULT_GLOBAL_CONFIG =  {
  telemetry: {
    enabled: true,
    audit: false,
    endpoint: "https://telemetry.agentcore.aws.dev",
  },
  installationId: crypto.randomUUID(),
} as GlobalConfig