diff --git a/.github/harness/prompts/review.md b/.github/harness/prompts/review.md index 7d4bfff13..f3740f951 100644 --- a/.github/harness/prompts/review.md +++ b/.github/harness/prompts/review.md @@ -54,6 +54,48 @@ all issues have already been flagged. ## Patterns to look out for +These are findings this repo's reviewers raise repeatedly. Check for them specifically; they are +cheap to miss by reading a diff in isolation. + - **Excessive mocking** - Avoid excessive mocking; it couples tests to implementation details, provides weaker guarantees, and often points to mismanaged dependencies. Prefer real dependencies (e.g. temp directories over fs mocks) and only mock at true I/O boundaries (e.g. network calls, AWS SDK clients, HTTP requests). +- **Comments that restate the code** - A comment earns its place only by adding what the code cannot + say: a reason, a constraint, a rejected alternative. Flag added comments that paraphrase the line + below them. Say which lines. +- **Tests that assert the implementation** - A test that would pass for any implementation of the + same signature, or that restates a literal the code already contains, is maintenance cost with no + regression caught. Conversely, flag a new flag or option whose **absent** case is untested: that + is the path a default or a middleware silently changes. +- **`command.md` in the diff** - It is generated and committed by the release workflow. Its presence + in a feature PR is unrelated churn; ask for it to be reverted. +- **Unexplained changes in re-recorded fixtures or golden files** - A re-record from another account + rewrites every ARN; that churn is expected. Flag changes beyond those identifiers that the PR does + not explain, and recordings that were hand-edited or reformatted rather than produced by + `RECORD=1`. +- **A per-run value inside an SDK request** - Fixture keys hash the request, so a `randomUUID()`, + `Date.now()`, or generated name built inside the handler rehashes every run and the command can + never replay. The generator belongs in `CoreClient`'s config so a test can pin it. Same for a + streaming response, which cannot be recorded until `src/testing/fixtures.tsx` supports it. +- **An error message that lost what it used to name** - When an error path is refactored, check the + replacement still identifies which item failed. Moving the identity into a structured field does + not excuse dropping it from the message the user reads. +- **Dead guards on resolved context values** - A middleware-resolved value such as `RegionKey` is + never `undefined` below the middleware that sets it, so a branch on its absence cannot run. This + reads as correct defensive code and is not. +- **Presentation reaching only one surface** - An error hint, warning, or suggestion added in a + screen is missing from the headless path, and vice versa. Check both, including under `--json`, + where silence can hide a no-op a script will not notice. +- **Presentation in `src/core/`** - Core imports the contract types a handler declares in its + `types.tsx`; it must not import screens, components, routers, or user-facing text from + `src/handlers/`. Hints and suggested next steps belong in the handler. +- **Free functions that should be methods** - Several helpers taking the same state as their first + argument belong on the type that owns it (`ProjectManager`, `SourceResolver`). +- **Presence checked instead of emptiness** - `x` existing as `[]` is not the feature being + configured; the test is usually `(x?.length ?? 0) > 0`. +- **Missing `{ sensitive: true }`** - On a credential-bearing flag, or on literal input sent to a + model or runtime at call time. A sensitive value must also cross into the TUI as a Context value, + never as a route segment or query param. +- **A deploy-path field with no backing** - A field expected to reach a deployed resource needs the + CloudFormation property, support in a released `@aws/agentcore-cdk`, and the pin bump in + `src/assets/cdk/package.json`. There is no `AwsCustomResource` escape hatch. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index fdd01da54..9231af480 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -28,11 +28,17 @@ How have you tested the change? - [ ] I ran `bun test` - [ ] I ran the relevant end-to-end tests with `bun run test:e2e`, or explained why they are not applicable + (CI does not run them for you) - [ ] I ran `bun run typecheck` - [ ] I ran `bun run lint:check` - [ ] I ran `bun run format:check` +- [ ] I ran `bun run audit` +- [ ] I ran `bun run secrets:check` - [ ] I ran `bun run build` - [ ] If I modified `src/assets/`, I updated affected snapshots with `bun test --update-snapshots` and committed them +- [ ] If I re-recorded fixtures or golden files, every change beyond account identifiers is explained + by this PR +- [ ] This PR does not change `command.md` (the release workflow regenerates it) ## Checklist diff --git a/AGENTS.md b/AGENTS.md index c500298f3..c2e2fa873 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,7 +48,14 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che - **Global config keys.** Adding a global config key means updating `types.tsx`, `DEFAULT_GLOBAL_CONFIG`, and `applyOverrides` in `src/globalConfig/`. Retired keys in an existing `~/.agentcore/config.json` are dropped on read (the zod schema strips unknown keys). -- **TUI support is opt-in per top-level command** via the root's `.supportedTuiCommands(...)`. +- **TUI support is opt-in per router, at any depth**, via that router's `.supportedTuiCommands(...)`; + `compile()` threads support down the tree, so a nested router hides its own leaves (24 routers under + `src/handlers/` declare one). Omitting a leaf keeps it command-line only. A branch's _own_ support is + not its decision: `Router.doesSupportTui()` always returns `true`, so whether a group launches the + TUI depends entirely on whether its parent's list names it. A new group its parent does not list + inherits no TUI dispatch; one the parent does list launches the TUI on a bare invocation even with no + screen of its own, and its `.default(...)` runs only when the TUI is declined — `--json`, no TTY, or + the parent leaving it out. Test a bare invocation of any new group for exactly this. Handler registration order is `--help` and menu order. Commands without TUI support are omitted from the TUI menu. `.menuSection(title)` draws a menu divider above the next registered handler (menu only). @@ -59,7 +66,15 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che hashed over the command `input` (top-level `clientToken` excluded). Any change to the request a handler sends (new field, different default, different region) makes replay fail with "Missing fixture ... Re-run with RECORD=1". Either re-record against live AWS or keep the request - unchanged. Tests pin `--region us-west-2` for stable recordings. Never hand-edit or reformat + unchanged. The fixture path depends only on the request, so a request sent several times — a + poll for a long-running job — records to one file, each call overwriting the last; replay serves + the final recorded response to every call. A key whose value is `undefined` is dropped before hashing (the key is + `JSON.stringify`'d), so a new optional field left unset keeps the old hash — a new optional flag is + replay-safe whenever it is absent. It also means **a per-run value inside the request makes the command permanently + unreplayable** — a `randomUUID()` session id or a timestamp rehashes every run. Inject the + generator through `CoreClient`'s config (defaulting to the real one) so tests can pin it. A + **streaming response** likewise cannot be recorded until `src/testing/fixtures.tsx` grows + freeze/revive support for it. Tests pin `--region us-west-2` for stable recordings. Never hand-edit or reformat fixtures/golden files (`__fixtures__` and `*.snap` are Prettier-ignored). Use `settle()` and `uniquePerRecording()` from `src/testing/fixtures.tsx` for service state transitions and per-recording unique values. @@ -69,13 +84,36 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che staged at build time from `@aws/agent-inspector` (files get a `.asset` suffix); `AGENT_INSPECTOR_PATH` overrides it with a local build. `scripts/sync-vended-cdk.ts` pins `@aws/agentcore-cdk` in `src/assets/cdk/package.json` (release workflow; must not import `src/`). -- **`command.md` is generated** from `agentcore --help` ("Do not edit directly"). Regenerate it - after changing command names, flags, or descriptions. Section grouping lives in - `DEFAULT_GROUPS` in `scripts/generate-command-reference.mjs`. +- **`command.md` is generated, and not yours to regenerate.** `release-prepare.yml` runs + `bun scripts/generate-command-reference.mjs` and commits the result in the release PR. Leave it + out of a feature PR: regenerating on a branch pulls in every command change merged since the + last release, and reviewers ask for it to be backed out. Generate locally to preview your + `--help`, then revert. Section grouping lives in `DEFAULT_GROUPS` in that script. - **Build quirks** (`scripts/build.ts`): `@aws-cdk/toolkit-lib` is external in the npm bundle but inlined in compiled binaries (so the bootstrap template is embedded explicitly); identifiers are never minified because stack traces and telemetry error names depend on them. `dist/index.js` is a loader that enables Node's compile cache then imports `dist/main.js`. +- **A context value a middleware resolves is never `undefined` below it.** `withRegion` at the root + replaces `RegionKey` with the resolved region (flag → env → config → `DEFAULT_REGION`) before any + handler runs, so `ctx.value(RegionKey)` is always set and a guard on its absence is dead code. To + tell "the user passed `--region`" from "a default was resolved", read `RegionExplicitKey`, which + `withRegion` sets before it resolves the region. For a new middleware-resolved value, capture the + same distinction before the middleware runs; don't infer it from the context value. **A zod + `.default()` on a flag hides the same distinction**: by the time `handle` runs, a defaulted flag + and a passed one are identical. If any rule depends on the user having passed it, declare the + schema `.optional()` and apply the default in `handle`, and put the default's value in the flag + description so `--help` still states it. +- **The e2e suite is not a PR gate.** `.github/workflows/e2e-test.yml` runs only on + `workflow_call` / `workflow_dispatch` and checks caller authorization, so nothing runs it for a + contributor automatically. Run it locally with credentials and say so in the PR. +- **A deploy-path field needs three things, in order**: the property published on the + `AWS::BedrockAgentCore::*` resource (there is no `AwsCustomResource` escape hatch), support in + a released `@aws/agentcore-cdk`, then the pin in + `src/assets/cdk/package.json` bumped. Check `src/core/project/backends/cdk.ts` for whether the + resource is mapped at all. +- **Check emptiness, not presence, on optional collections.** `spec.efsAccessPoints` existing as + `[]` is not the same as the feature being configured; test `(spec.efsAccessPoints?.length ?? 0) > 0`, + as `src/core/project/templates/export.ts` does. - Line endings are LF (`.gitattributes`). The Husky pre-commit hook runs lint-staged (oxlint --fix + Prettier) on staged files. - Root-level `.*.bun-build` files are compile leftovers (gitignored); ignore them. @@ -86,15 +124,25 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che - tsconfig is strict with `noUncheckedIndexedAccess` and `verbatimModuleSyntax` (use `import type` for type-only imports). Relative imports often include the extension (`./harness/index.tsx`), and most files are `.tsx` even without JSX; follow the neighbouring files. -- Handler directories: `index.tsx` (`createHandler(core, io)` factory, re-exports the - screen), `screen.tsx`, `types.tsx` (the Core interface the handler consumes; implemented in - `src/core/`). Leaf handlers use `createHandler({ name, description, flags: [flag(...)], handle })` - with zod-validated flags. Emit JSON via `ctx.require(JsonRendererKey).renderJson(...)` and write - through the injected `AppIO`; never touch `process.stdout` directly in handlers. +- Handler directories: `index.tsx` (`createHandler(core, io)` factory), `screen.tsx` (imported + directly by the route table in `src/components/Root.tsx`), `types.tsx` (the Core interface the + handler consumes; implemented in `src/core/`). Leaf handlers use + `createHandler({ name, description, flags: [flag(...)], handle })` with zod-validated flags. Emit + JSON via `ctx.require(JsonRendererKey).renderJson(...)` and write through the injected `AppIO`; + never touch `process.stdout` directly in handlers. - Build `CoreOptions` from context with `coreOptsFromCtx(ctx)` (`src/handlers/utils.tsx`). +- A leaf that prints human-readable output unless `--json` is passed reads `ctx.value(JsonKey)` + (`src/handlers/keys.tsx`) to choose; 29 handlers do. - Errors: throw `AgentCoreCLIError` subclasses (`src/errors/`), which carry `source` (user/service/internal), `exitCode`, and `meta` for logging and telemetry. `AgentCoreCLIError.fromError` maps Commander errors to exit code 2 and SDK 4xx to user errors. + `SilentCLIError` (`src/errors/errors.tsx`) is the one to throw when the command has already + written its own output and only needs to carry an exit code — a remote command's exit status, for + instance — so the root handler does not print a second message over it. + Error text is asserted behaviour: when you refactor an error path, keep everything the old + message named. If it identified which item failed, the replacement must still identify it — + moving the identity into a structured field does not excuse dropping it from the sentence the + user reads. - Runtime files: logs in `~/.agentcore/logs/`, global CLI settings in `~/.agentcore/config.json`. - Commits and PR titles use conventional commits with scope: `feat(invoke): ...`, `fix(tui): ...`, `refactor(gateway): ...`, `test(cli): ...`. PRs must link an issue and follow @@ -102,7 +150,19 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che ## Testing patterns -- Tests live beside code as `*.test.ts(x)`; target is 90% line coverage. +- Tests live beside code as `*.test.ts(x)`; name any test-support file `*-test-support.ts` (the glob + the oxlint override in `.oxlintrc.json` matches) so it is not mistaken for shipped code. Target is + 90% line coverage, but coverage is a floor on behavior tested, not a quota to fill — a test + asserting what the implementation plainly says gets removed in review. For a flag, the case worth + testing is usually the one where it is **absent**, since that is the path a default or a + middleware silently changes. +- `testIO().stdout()` / `.stderr()` return the captured text with the trailing newline trimmed, so + assert `toBe("hello")`, not `toBe("hello\n")`. A new helper or constant in `src/testing/` is only + visible to tests once it is re-exported from the `src/testing/index.tsx` barrel. +- **Client tests** go one layer lower, for assertions about the request a Core client builds: + `new EvalClient({ data: () => ({ send: mock(fn) }) as never } as unknown as AwsClients)`, then assert + the SDK command's `input`. No fixture, no recording — the right choice when the point is the request + itself. Several `src/core/*.test.tsx` files use it. - **Command-flow tests** build the real stack: `new CoreClient({ ...fixtureFactories(FIXTURES), logger: createSilentLogger() })`, then `createRootHandler(core, { io: testIO().io, ... })`, then `root.route(["node", "agentcore", ...args, "--region", REGION])`, asserting captured stdout with @@ -115,4 +175,6 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che - Pass `platform: "win32"` to `createRootHandler` or `renderScreen` to exercise Windows branches; CI runs unit tests on Linux, Windows, and macOS. - Shared helpers are exported from `src/testing/index.tsx` (`testIO`, `expectError`, - `createSilentLogger`, `TestGlobalConfigAccessor`, fixture helpers). + `createSilentLogger`, `TestGlobalConfigAccessor`, fixture helpers including `fixtureFetch` for + non-SDK HTTP). `menuEntries` in `src/testing/renderScreen.tsx` reads a rendered TUI menu, + split at its command-line-only divider. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8d911f708..e54bc1519 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -17,16 +17,52 @@ Run development commands from the repository root. out of the PR. Use conventional commit messages such as `fix:`, `feat:`, or `docs:`. - **Add or update tests.** Cover behavior changes and add a regression test for bug fixes. Update affected fixtures and snapshots. Do not disable tests or remove assertions to make checks pass. -- **Run the checks.** Unit tests, typechecking, linting, formatting, and the build must pass: - `bun test`, `bun run typecheck`, `bun run lint:check`, `bun run format:check`, and `bun run build`. + Test observable behavior, not implementation detail: a test that only restates what the code + does adds maintenance cost without catching a regression, and reviewers will ask for it to go. +- **Run the checks.** Unit tests, typechecking, linting, formatting, the dependency and secret + scans, and the build must pass: `bun test`, `bun run typecheck`, `bun run lint:check`, + `bun run format:check`, `bun run audit`, `bun run secrets:check`, and `bun run build`. Run the relevant [end-to-end tests](e2eTest/README.md) for changes to deployment or AWS interactions. All required CI checks must pass before merge. - **Keep the documentation current.** Update affected commands, examples, and configuration - guidance. Call out breaking changes and any migration steps in the PR. + guidance in `README.md` and `docs/`. Call out breaking changes and any migration steps in the PR. + Do not regenerate `command.md` — see [Generated files](#generated-files). +- **Comment why, not what.** A comment earns its place by adding what the code cannot say for + itself: the reason behind a non-obvious choice, a constraint, or why an alternative was rejected. + Delete comments that restate the line below them, and prefer a clearer name over an explanation. - **Report what you verified.** List the checks you ran and their results. State explicitly when a test could not be run and why. Do not include credentials or customer data in code, fixtures, or logs. +## Generated files + +Two files in the tree are generated. Neither is hand-edited, and they are refreshed at +different times: + +- **`command.md`** is generated from `agentcore --help`. The release workflow + (`.github/workflows/release-prepare.yml`) regenerates it and commits it in the release PR, + so **leave it out of a feature PR.** Regenerating it on a feature branch picks up every + other command change merged since the last release, which buries your diff in unrelated + churn. Run `bun scripts/generate-command-reference.mjs` locally if you want to preview how + your `--help` will read, then revert the file before committing. +- **Fixtures and golden files** under `src/handlers/**/__fixtures__/` and `src/core/**/__fixtures__/` are recordings. Refresh + them with `RECORD=1 bun test ` — see [Golden files and record mode](#golden-files-and-record-mode). + +## Changes that reach a deployed resource + +The CLI vends CDK projects built from `@aws/agentcore-cdk`, pinned in +`src/assets/cdk/package.json`. A change that has to reach a deployed resource lands in three +steps, in order: + +1. The CloudFormation resource must publish the property. The CLI deploys the control plane + only through CloudFormation and has no escape hatch, so a property absent from the resource + schema is one the CLI cannot set. +2. A released `@aws/agentcore-cdk` supports it. +3. The pin in `src/assets/cdk/package.json` is bumped to that release. + +Say in your issue when a change needs steps 1 or 2, so maintainers can sequence them. Expect the +pin bump to be a follow-up rather than part of the same change. + ## Development Install [Bun](https://bun.com). @@ -321,8 +357,9 @@ Conventions: - **`index.tsx`** exports a `createHandler(...)` factory returning a `Handler`/`Router`. Dependencies (such as `Core` and `AppIO`) are passed in, - never imported as singletons. Re-export the command's `screen.tsx` from here. -- **`screen.tsx`** exports the React component(s) for the TUI. Screens receive + never imported as singletons. +- **`screen.tsx`** exports the React component(s) for the TUI, which `src/components/Root.tsx` + imports directly when it routes them. Screens receive `ScreenProps` (`{ ctx, core }`) threaded down from `Root`, and drive data fetching with react-query against `core`. - **`types.tsx`** defines the interface(s) this command needs from Core. @@ -336,6 +373,89 @@ Conventions: Mount the new handler in `src/handlers/index.tsx` (or on the appropriate parent router), passing its dependencies, as in `root.handler(createHarnessHandler(core, io))`. +When two or more commands take the same set of flags, put the set on a class with +`static readonly flags` (an `as const` array each command spreads into its own `flags`) and a +`static resolve(flags, …)` that owns every rule across those flags. +`src/handlers/eval/sessionSource.tsx` is the model. Keeping the flags and their validation on +one type means a command cannot take the flags without the rules, and spreading the array keeps +`--help` order identical across the commands that share it. A rule involving a flag outside the +set belongs in the command that owns that flag, not on the class. Only flags that are identical across the +commands — same name, schema, required-ness and description — go on the class; when the overlap is +partial, the flags that differ stay on each command. + +`flag(name, description, schema, { group, help })` takes two optional extras that shape `--help`: +`group` is the heading the flag is listed under, and `help` is long-form text printed in a +`Parameter details:` section. Keep long `help` in a sibling file named for what it documents — +`src/handlers/eval/online-eval/filtersHelp.tsx` and `harness/parameterHelp.tsx` are the models — and +follow their house style: a one-line summary, the JSON shape with `// [required]` annotations, a link +to the API reference, and two worked examples. A flag taking tags goes through `parseTags` +(`src/handlers/utils.tsx`), which accepts one JSON object or repeated `key=value` entries and +validates both against `TagsSchema`; do not hand-roll tag parsing. + +### What a change touches beyond its handler + +Work this out before editing anything. Missing one of these is the most common reason a PR +needs a second review round. + +**A new flag, field or output value on an existing command:** + +| File | Change | When | +| ---------------------------------------- | --------------------------------------------------------- | ----------------------------------------- | +| `src/handlers///index.tsx` | add to the `flags: [flag(...)]` array with its zod schema | always | +| `src/handlers//types.tsx` | widen the Core interface the handler consumes | if the value reaches the SDK | +| `src/core/.tsx` | put the value into the SDK request | if the value reaches the SDK | +| `src/projectSchemas/.ts` | add the field to the zod project schema | only on an `agentcore add` path | +| `src/core/project/backends/cdk.ts` | map it to the deployed resource | only if it must reach a deployed resource | +| `src/testing/TestCoreClient.tsx` | add the new method to the matching test sub-client | if a Core interface gained a method | +| the handler's `*.test.tsx` | a command-flow case covering the new value | always | +| `__fixtures__/` and `*.golden.json` | re-record with `RECORD=1` | if the SDK request changed | +| `README.md`, `docs/` | update the guidance that covers it | if it exists | + +**A new leaf** adds its own directory (`index.tsx`, `screen.tsx` if it has a TUI screen), +and must be mounted on its parent router — registration order is `--help` order. Add it to +the parent's `.supportedTuiCommands(...)` only if it has a screen; mutation leaves ship +headless first, and the TUI menu lists them under its command-line-only divider. The +domain's test suite usually asserts the router's exact child list and menu entries (and +`src/handlers/root.test.tsx` does for top-level commands), so update those assertions too. + +**A new Core sub-client** — a domain Core did not talk to before — is declared on the `Core` +type in `src/handlers/types.tsx`, constructed in `CoreClient` (`src/core/index.tsx`), and +mirrored by a test sub-client in `src/testing/TestCoreClient.tsx`, which implements `Core` +and so fails typecheck until it matches. + +**A new resource with a project collection** also needs its `src/projectSchemas/.ts` +schema registered in `project.ts` with `.superRefine(uniqueNames(""))`, a Core +sub-client, a deploy mapping in `src/core/project/backends/cdk.ts`, and support in a released `@aws/agentcore-cdk` (see +[Changes that reach a deployed resource](#changes-that-reach-a-deployed-resource)). Add a +`type: z.literal(...)` discriminator only if the sibling schemas have one — most do not. + +**Removing a flag, field or return value** runs the same inventory in reverse: grep for every +reference, including the domain's `types.tsx` interface, `TestCoreClient`, test assertions and +`README.md` examples, and delete exported helpers the removal leaves with no caller outside their own +tests. + +**A leaf that opens its own screen from flags** — `gateway invoke --id X` landing straight on +that gateway's screen — calls `renderTuiAt` with `${ctx.require(PathKey)}/${encodeURIComponent(id)}`, +and takes the renderer as an injectable parameter defaulting to `renderTuiAt` so a test can assert +the path without a terminal. `src/handlers/gateway/invoke/index.tsx` is the model. Once a partial set +of flags opens the TUI, the existing headless validation tests for that leaf need `--json`, or they +open the TUI instead of failing. + +**A Core call a screen can leave while it is still running** — a long poll, a stream, a +generation the user cancels with Esc — takes `signal?: AbortSignal` as its last parameter, as ten +Core methods across `eval`, `gateway`, `harness` and `runtime` already do. The screen owns an +`AbortController` and aborts on Esc and on unmount. If the method you need does not take a signal +yet, extend it: ignoring a late result while the service call keeps running leaves work in flight +that the user asked to stop. + +**A new TUI screen** must also be routed: add its `` entries to the route table in +`src/components/Root.tsx` and import it there, or the screen is unreachable however correct it is. +A bare parent path (`…/get` with no id) gets a `Navigate` to the list, which is what +`withTuiOnEmptyFlagsAndArgs` lands on. The screen uses `useWindowSize` from +`src/components/ui/useWindowSize.tsx`, puts a wrapper shared by more than one screen in +`src/components/`, and needs a screen test through `renderScreen`. A new primitive under `src/components/ui/` is a larger change than it looks; +raise it before building one. + ### Core and dependency inversion AWS operations and project orchestration live in **`src/core/`**, behind @@ -358,6 +478,17 @@ it takes **factory functions** (`(config) => new BedrockAgentCore...Client(...)` injected at the app edge in `src/index.ts`. That keeps the SDK swappable — crucial for the testing strategy below. +The import direction follows from this. **Core imports the contracts the handlers declare +— the types in `src/handlers//types.tsx` — and nothing that presents.** It never +imports a screen, a component, a router, or user-facing text. When something in Core needs +presentation — an error hint, a suggested next command — it belongs in the handler instead. Put it where every caller reaches it, though: a hint +added only to a screen is missing from the headless path, which is where most users hit the +error. + +Loose helper functions that all operate on the same state belong on the type that owns it +(`ProjectManager`, `SourceResolver`) rather than as a module of free functions passed the +same argument. Reserve free functions for genuinely stateless utilities. + ### The TUI The interactive UI is built with [Ink](https://github.com/vadimdemedes/ink) @@ -385,9 +516,14 @@ We aim for **90% line coverage** (`bun test --coverage`). #### Injected IO +A command with live output does not go through the JSON renderer: it writes the bytes to +`io.stdout` as they arrive and keeps progress, banners and session metadata on `io.stderr`, so a +caller can redirect one without the other. `--json` suppresses the streaming and emits one folded +object instead. + Handlers and screens use injected streams for user-facing output. An `AppIO` (`{ stdin, stdout, stderr }`, defined in `src/io/types.ts`) is passed in the -configuration object to `createRootHandler(core, { io, logger, globalConfigAccessor, globalConfig })` +configuration object to `createRootHandler(core, { io, logger, globalConfigAccessor })` at the edge (`src/index.ts` passes the real process streams) and threaded down to the TUI renderer and handlers. JSON output flows through the context: a `withJsonRenderer` middleware pins a `JsonRenderer` wired @@ -403,6 +539,9 @@ compare rendered output against committed **golden files**. The record/replay seam sits at the SDK `.send()` boundary (the same seam `src/index.ts` wires the real clients into), so replayed tests still exercise the real `CoreClient`, `HarnessClient`, and option translation — only the network call is swapped out. +Calls that are not AWS SDK operations — a presigned URL, a non-AWS HTTP service — go +through the `fetch` injected into `CoreClient`, and record and replay the same way through +`fixtureFetch(dir)`. `src/handlers/feedback/feedback.fixture.test.tsx` is the model. Two modes, selected by the `RECORD` env var: @@ -418,6 +557,76 @@ and strip volatile transport metadata (`$metadata`, request IDs) so they stay stable. Golden files are excluded from Prettier (`.prettierignore`) — they are byte-for-byte recordings, not source to reformat. +Record in your own AWS account. Any account works; maintainers record against the project's own +account, so a recording may be re-made later with different identifiers in it. + +### Setting up an account to record + +1. **Credentials.** Export credentials the usual way (`AWS_PROFILE`, or the standard environment + variables). The suites use the default AWS credential chain. +2. **Region.** Every fixture-backed suite pins `--region us-west-2`, so AgentCore must be available + to your account in `us-west-2`. +3. **Permissions.** The identity needs the AgentCore control- and data-plane operations the suite + calls, plus any IAM, CloudWatch Logs or CloudFormation calls it makes. The SDK commands a suite + sends are the fixture file names in its `__fixtures__/` directory. +4. **Pre-existing resources.** Many suites read a resource that must already exist, pinned in a + `FIXTURE_*` constant at the top of the test file, whose header comment says what it needs. + Create the equivalent in your account and point the constant at it. +5. **Record.** Run `RECORD=1 bun test `, then review the diff before committing. + +Before you record one: + +- **Recordings carry the recording account's identifiers, and that is expected.** Most committed + fixtures and golden files contain real account IDs in their ARNs; `matchGolden` strips presigned + URLs and normalizes JSON, nothing else. A re-record from a different account therefore rewrites + every ARN in the suite. Review the diff for changes beyond those identifiers. +- **A value the service needs fresh on every recording goes through `uniquePerRecording`** + (`src/testing/fixtures.tsx`). Record mode generates a new value and saves it; replay reads the + saved one back, so it is identical on every replay and different on every recording — a session + id that must not collide with an earlier recording's traces, for instance. Call it inside the + test, not at module load, or replay fails the whole file when the value has never been recorded. +- **The two failure messages mean different things.** `Missing fixture …` means the SDK + request your handler sends has changed, because fixture keys hash that request — either + re-record or leave the request alone. `Missing golden file …` just means this assertion + has never been recorded; record it. +- **Build a fresh `CoreClient` per run.** It carries per-run caches, so sharing one across + tests leaks state between them. See the `createFixtureCore()` helper in + `src/handlers/harness/harness.test.tsx`. +- **A new AWS client needs a new fixture factory.** `fixtureFactories` returns one factory + per client the CLI talks to (CloudFormation, AgentCore control and data, IAM, CloudWatch + Logs, X-Ray, Application Signals). A handler reaching a service that is not in that list + cannot replay until the factory is added. + +A golden file records what a user sees, so review it like output, not like a blob: if the +diff is not explainable by your change, something else moved. + +A test that hand-builds part of an SDK response casts it `as unknown as `, the idiom +across the suite. A bare `as ` on a partial literal compiles only until the literal +contains a nested optional, then fails comparability in both directions with TS2352. + +### Two things that make a command unrecordable until the harness is extended + +Check both against the operation you are adding **before** writing the handler. Finding out +afterwards means reworking `CoreClient`'s constructor or the test harness, not just your +command. + +- **A value generated per run inside the SDK request.** Fixture keys hash the request, so a + fresh `randomUUID()` session id, timestamp, or generated name produces a fresh hash on + every run and the fixture can never be found again. Inject the generator at the Core edge + — `CoreClient` takes it as config, defaulting to the real implementation — so a test can + pin it. A handler that calls the generator directly cannot be replayed at all. +- **A streaming response.** `makeRecordingSend` cannot serialise a stream, so there is + nothing to record. The operation needs freeze-on-record / revive-on-replay support in + `src/testing/fixtures.tsx` before a test over it can exist. + +### Which test to write + +Prefer a fixture-backed command-flow test over a unit test when the command's value is its +rendered output. A unit test over a Core helper asserts the shape you just wrote; the +command-flow test asserts what the customer sees, through the real parser, middleware, Core +and renderer. When you add command-flow coverage, delete the unit test it replaces rather +than keeping both. + See [this talk](https://www.youtube.com/watch?v=yszygk1cpEc&t=1s) for background on the pattern.