From 4bf7f16bbb2848cb707fc3ac022fa3277e64b289 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 16:28:11 +0000 Subject: [PATCH 01/14] docs: close gaps in the contribution standards Four standards files drift from what CI enforces and what reviewers ask for. Each change below is verifiable in the repo: CONTRIBUTING.md - Add `bun run audit` and `bun run secrets:check` to the required checks; both are gates in verify.yml but were missing from the list, so a contributor running the documented five still fails CI. - New "Generated files" section: command.md is regenerated and committed by release-prepare.yml, so it should stay out of feature PRs. - New "Changes that span repositories" section: the CloudFormation property -> released L3 construct -> @aws/agentcore-cdk pin order. - Four golden-file traps: account-specific values survive recording, the two distinct "Missing fixture" / "Missing golden file" causes, the fresh CoreClient per run, and extending fixtureFactories for a new AWS client. - State the one-way import direction (src/core must not import src/handlers), and that presentation added in a screen misses the headless path. - Ask for behavior tests over implementation tests, and for comments that give a reason rather than restate the line below them. AGENTS.md - Correct the command.md entry: it told contributors to regenerate the file, which pulls in every command change merged since the last release. - Add: a middleware-resolved context value is never undefined below it, so a guard on its absence is dead code. - Add: the e2e suite is not a PR gate (workflow_call / workflow_dispatch only). - Add: the three steps a deploy-path field needs, and emptiness-vs-presence on optional collections. - Note that coverage is a floor on behavior tested, and that a new flag's absent case is the one worth a test. .github/pull_request_template.md - Add the two missing check boxes, a companion-repo section, and boxes for the two mistakes that cost a review round: account-specific values in recordings and command.md churn. .github/harness/prompts/review.md - Extend the patterns list from one entry to the findings reviewers raise repeatedly, so they are caught before a human spends a round on them. --- .github/harness/prompts/review.md | 33 +++++++++++++ .github/pull_request_template.md | 12 +++++ AGENTS.md | 29 ++++++++++-- CONTRIBUTING.md | 77 +++++++++++++++++++++++++++++-- 4 files changed, 144 insertions(+), 7 deletions(-) diff --git a/.github/harness/prompts/review.md b/.github/harness/prompts/review.md index 7d4bfff139..e8a7fb7535 100644 --- a/.github/harness/prompts/review.md +++ b/.github/harness/prompts/review.md @@ -54,6 +54,39 @@ 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. +- **Account-specific values in fixtures or golden files** - Account IDs, ARNs, or generated resource + names recorded from the author's own account do not reproduce elsewhere. Hand-edited or + reformatted recordings are also a finding: they must come from `RECORD=1`. +- **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. +- **`src/core/` importing from `src/handlers/`** - The dependency runs one way only. Core implements + contracts handlers declare. +- **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, the released L3 construct, and the `@aws/agentcore-cdk` 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 fdd01da546..0003ac1095 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -12,6 +12,12 @@ Closes # +## Related Changes in Other Repositories + + + ## Type of Change @@ -28,11 +34,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, the diff contains no account IDs, ARNs, or other + values specific to the account I recorded against +- [ ] This PR does not change `command.md` (the release workflow regenerates it) ## Checklist diff --git a/AGENTS.md b/AGENTS.md index c500298f36..69c9cb8869 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -69,13 +69,30 @@ 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 `--flag`" from "something resolved a default", capture that + before the middleware runs — don't infer it from the context value. +- **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), the L3 + construct released from `aws/agentcore-l3-cdk-constructs`, then the `@aws/agentcore-cdk` 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.** `config.insights` existing as `[]` + is not the same as the feature being configured; test `(x?.length ?? 0) > 0`. - 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. @@ -102,7 +119,11 @@ 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` 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. - **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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8d911f708e..b69790d875 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__/` are recordings. Refresh + them with `RECORD=1 bun test ` — see [Golden files and record mode](#golden-files-and-record-mode). + +## Changes that span repositories + +The CLI vends CDK projects built from [`@aws/agentcore-cdk`](https://github.com/aws/agentcore-l3-cdk-constructs), +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. The L3 construct change merges and is released in `aws/agentcore-l3-cdk-constructs`. +3. The pin in `src/assets/cdk/package.json` is bumped to that release. + +Link the companion PR from this one, and expect the pin bump to be a follow-up rather than part +of the same change. + ## Development Install [Bun](https://bun.com). @@ -358,6 +394,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 and is one-way: **`src/core/` must not import from +`src/handlers/`.** Core implements contracts the handlers declare; it does not know they +exist. 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) @@ -418,6 +465,30 @@ 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. +Four things to know before you record one: + +- **Scrub account-specific values out of the recording.** `matchGolden` strips presigned + URLs and normalizes JSON, but nothing else. An account ID, an ARN, or a generated + resource name recorded from your own account is committed verbatim and will not + reproduce on anyone else's, so the next person to re-record sees a diff with no + behavior change behind it. Use `uniquePerRecording` (`src/testing/fixtures.tsx`) for + values that must stay stable across re-records, and keep account-derived identifiers + out of asserted output. +- **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. + See [this talk](https://www.youtube.com/watch?v=yszygk1cpEc&t=1s) for background on the pattern. From 036efba9146b0b937e01a952c9b278eb4741588e Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 17:11:10 +0000 Subject: [PATCH 02/14] docs: document unrecordable requests and the per-change file inventory CONTRIBUTING.md - New "What a change touches beyond its handler": the files a new flag, leaf, resource or screen touches, so a contributor inventories the change before editing. - New subsection on the two conditions that make a command unrecordable until the test harness is extended: a per-run value (session id, timestamp) inside the SDK request, which rehashes the fixture key every run, and a streaming response, which makeRecordingSend cannot serialise. - "Which test to write": prefer the fixture-backed command-flow test over a unit test of a Core helper, and delete the unit test it replaces. AGENTS.md - Extend the fixture-hashing gotcha with the per-run-value and streaming cases. - Error text is asserted behaviour: a refactored error path must still name what the old message named. .github/harness/prompts/review.md - Add both as review patterns. --- .github/harness/prompts/review.md | 7 ++++ AGENTS.md | 10 +++++- CONTRIBUTING.md | 57 +++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/.github/harness/prompts/review.md b/.github/harness/prompts/review.md index e8a7fb7535..3db62a9e26 100644 --- a/.github/harness/prompts/review.md +++ b/.github/harness/prompts/review.md @@ -72,6 +72,13 @@ cheap to miss by reading a diff in isolation. - **Account-specific values in fixtures or golden files** - Account IDs, ARNs, or generated resource names recorded from the author's own account do not reproduce elsewhere. Hand-edited or reformatted recordings are also a finding: they must come from `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. diff --git a/AGENTS.md b/AGENTS.md index 69c9cb8869..de8a457846 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,7 +59,11 @@ 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. 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. @@ -112,6 +116,10 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che - 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. + 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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b69790d875..86c77df2ab 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -372,6 +372,40 @@ 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))`. +### 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 | +| 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. + +**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 the companion L3 +construct (see [Changes that span repositories](#changes-that-span-repositories)). Add a +`type: z.literal(...)` discriminator only if the sibling schemas have one — most do not. + +**A new TUI 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 @@ -489,6 +523,29 @@ Four things to know before you record one: 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. +### 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. From 50bae16e8269f1aac6abfa392352b8ab8534e94d Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 17:39:09 +0000 Subject: [PATCH 03/14] docs: correct the core import rule and document the fetch seam - Correct the core/handlers rule. Core type-imports the contracts each handler declares in its types.tsx; the rule is that it imports nothing that presents (screens, components, routers, user-facing text), not that it imports nothing. - Fix the createRootHandler options in CONTRIBUTING.md: RootHandlerConfig takes io, logger and globalConfigAccessor; there is no globalConfig option. - Document that non-SDK HTTP goes through the fetch injected into CoreClient and records and replays through fixtureFetch(dir). - Add TestCoreClient to the per-change file table: it implements Core, so a new Core method fails typecheck until the test sub-client has it too. - New-leaf and new-sub-client checklists: update the exact child-list and menu assertions (root.test.tsx for top-level commands), and wire a new sub-client through Core, CoreClient and TestCoreClient. - List fixtureFetch and menuEntries among the shared test helpers. --- .github/harness/prompts/review.md | 5 +++-- AGENTS.md | 4 +++- CONTRIBUTING.md | 23 +++++++++++++++++------ 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/.github/harness/prompts/review.md b/.github/harness/prompts/review.md index 3db62a9e26..acf79537aa 100644 --- a/.github/harness/prompts/review.md +++ b/.github/harness/prompts/review.md @@ -85,8 +85,9 @@ cheap to miss by reading a diff in isolation. - **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. -- **`src/core/` importing from `src/handlers/`** - The dependency runs one way only. Core implements - contracts handlers declare. +- **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 diff --git a/AGENTS.md b/AGENTS.md index de8a457846..0f11726732 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -144,4 +144,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 86c77df2ab..e9e21e6bd3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -386,6 +386,7 @@ needs a second review round. | `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 | @@ -393,7 +394,14 @@ needs a second review round. **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. +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 @@ -428,10 +436,10 @@ 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 and is one-way: **`src/core/` must not import from -`src/handlers/`.** Core implements contracts the handlers declare; it does not know they -exist. 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 +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. @@ -468,7 +476,7 @@ We aim for **90% line coverage** (`bun test --coverage`). 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 @@ -484,6 +492,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: From aaec8d2158cd72c9da97a76a8f2de635e7e9b897 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 17:55:04 +0000 Subject: [PATCH 04/14] docs: TUI route registration, flag help options, and two testing notes All four verified against this branch before writing: - Correct the supportedTuiCommands scope. AGENTS.md said it was opt-in "per top-level command via the root's" call; 24 routers under src/handlers declare their own, and compile() threads support down the tree. - Document that a new screen must be routed in src/components/Root.tsx, with a Navigate for the bare parent path. Without it the screen is unreachable, and the new-TUI-screen checklist never mentioned the file. - Document flag()'s group and help options, the sibling *Help.tsx convention for long-form flag help (filtersHelp.tsx, parameterHelp.tsx), and that a tags flag goes through parseTags, which validates against TagsSchema. - A zod .default() on a flag hides "passed" from "defaulted" by the time handle runs, the same trap AGENTS.md already documents for middleware context values. Declare .optional() and default in handle when a rule depends on the flag being passed. - Name client tests as a test kind: a mocked send() asserting the SDK request a Core client builds, with no fixture and no recording. --- AGENTS.md | 14 ++++++++++++-- CONTRIBUTING.md | 18 +++++++++++++++--- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0f11726732..9fd11c387f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,7 +48,9 @@ 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. 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). @@ -86,7 +88,11 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che 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 `--flag`" from "something resolved a default", capture that - before the middleware runs — don't infer it from the context value. + 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. @@ -132,6 +138,10 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che 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. +- **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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e9e21e6bd3..7b3150045e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -372,6 +372,15 @@ 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))`. +`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 @@ -409,9 +418,12 @@ sub-client, a deploy mapping in `src/core/project/backends/cdk.ts`, and the comp construct (see [Changes that span repositories](#changes-that-span-repositories)). Add a `type: z.literal(...)` discriminator only if the sibling schemas have one — most do not. -**A new TUI 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; +**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 From 6cbe6727e0d30dea35661c08a1d63040272f38f4 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 17:57:13 +0000 Subject: [PATCH 05/14] docs: a branch's TUI launch is its parent's decision, not its own - Router.doesSupportTui() always returns true (src/router/router.tsx:459), so whether a group launches the TUI depends only on whether its parent's .supportedTuiCommands(...) names it. A listed group 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 omitting it). Test a bare invocation of any new group for this. - Partial SDK response literals in tests cast `as unknown as `, the idiom used across the suite; a bare `as ` compiles until the literal gains a nested optional, then fails comparability with TS2352. --- AGENTS.md | 7 ++++++- CONTRIBUTING.md | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 9fd11c387f..e852188bf9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,7 +50,12 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che `~/.agentcore/config.json` are dropped on read (the zod schema strips unknown keys). - **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. + `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). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7b3150045e..8309d65e8b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -546,6 +546,10 @@ Four things to know before you record one: 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 From fc436c7f49c15e8378efe72436582fabbe5761ae Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 18:07:03 +0000 Subject: [PATCH 06/14] docs: SilentCLIError, streaming output, and two test-harness facts - Name SilentCLIError (src/errors/errors.tsx): throw it when the command has already written its own output and only needs to carry an exit code, so the root handler does not print a second message over it. - Document the live-output path. A streaming command writes bytes to io.stdout as they arrive and keeps progress and metadata on io.stderr; --json suppresses streaming and emits one folded object. The conventions previously read as though every command emits through the JSON renderer. - testIO().stdout()/.stderr() trim the trailing newline, so assertions compare against the untrailed string. - A new helper in src/testing/ is invisible to tests until re-exported from the src/testing/index.tsx barrel. --- AGENTS.md | 6 ++++++ CONTRIBUTING.md | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index e852188bf9..28e28c7c68 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -127,6 +127,9 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che - 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 @@ -143,6 +146,9 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che 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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8309d65e8b..cdc857e28c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -486,6 +486,11 @@ 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 })` From 618ab365c8a539aceb16c78789794c7d0ced6cfa Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 18:30:31 +0000 Subject: [PATCH 07/14] docs: recording in your own account, and shared flag groups - State that a contributor adding a flag to a fixture-backed suite re-records it with RECORD=1 under their own credentials and scrubs account-specific values before committing. The docs previously said both "prefer fixture-backed tests" and "re-record or leave the request unchanged" without saying whose account records. - Document the shared-flag-group convention: a class with `static readonly flags` spread into each command and a `static resolve(...)` owning the rules across those flags, modelled on src/handlers/eval/sessionSource.tsx. A rule involving a flag outside the set belongs in the command that owns that flag. --- CONTRIBUTING.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cdc857e28c..0ba081f7ee 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -372,6 +372,14 @@ 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. + `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 — @@ -527,6 +535,10 @@ 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 against your own AWS account. A contributor adding a flag to a command with a +fixture-backed suite re-records that suite with `RECORD=1` using their own credentials, then +scrubs the result before committing — the first point below is what makes that safe. + Four things to know before you record one: - **Scrub account-specific values out of the recording.** `matchGolden` strips presigned From 9c270d0a2d2e993e83dc6dc03a2f5c6abc7fb204 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 18:37:20 +0000 Subject: [PATCH 08/14] docs: undefined fields and the fixture hash, and a removal inventory - Fixture keys are JSON.stringify'd before hashing, which drops keys whose value is undefined. A new optional field left unset therefore keeps the old hash, so a new optional flag is replay-safe whenever it is absent. - The per-change inventory covered only additions. Add the reverse case: removing a flag, field or return value means finding every reference (types.tsx, TestCoreClient, test assertions, README examples) and deleting exported helpers the removal leaves without a caller. --- AGENTS.md | 4 +++- CONTRIBUTING.md | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 28e28c7c68..196f4037bc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,7 +66,9 @@ 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. It also means **a per-run value inside the request makes the command permanently + unchanged. 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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0ba081f7ee..147fb04f07 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -426,6 +426,11 @@ sub-client, a deploy mapping in `src/core/project/backends/cdk.ts`, and the comp construct (see [Changes that span repositories](#changes-that-span-repositories)). 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 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 From 9bad99fe858c417f47195157a7d5f3fbebb13a0e Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 18:40:55 +0000 Subject: [PATCH 09/14] docs: screens are imported by Root.tsx, and document leaf deep links - Drop the instruction to re-export screen.tsx from the handler's index.tsx. Only 32 of the 91 handler directories with a screen do it; Root.tsx imports 114 screens directly from their screen.tsx. State that instead. - Document the leaf deep-link pattern, used by 14 handlers: renderTuiAt with `${ctx.require(PathKey)}/${encodeURIComponent(id)}`, the renderer injected with a renderTuiAt default so tests can assert the path, modelled on gateway/invoke. Once partial flags open the TUI, that leaf's existing headless validation tests need --json. --- AGENTS.md | 4 ++-- CONTRIBUTING.md | 12 ++++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 196f4037bc..3aad820447 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -120,8 +120,8 @@ 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 +- 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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 147fb04f07..9b59299c2c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -357,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. @@ -431,6 +432,13 @@ reference, including the domain's `types.tsx` interface, `TestCoreClient`, test `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 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 From 837b05dc27b7582bbc93b591ae860286761efd15 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 18:45:17 +0000 Subject: [PATCH 10/14] docs: Core calls a screen can cancel take an AbortSignal Ten Core methods across eval, gateway, harness and runtime take `signal?: AbortSignal` as their last parameter, and the screens that call them own an AbortController that aborts on Esc and on unmount. Neither standards file mentioned the pattern. Document it, and say to extend a method that lacks a signal rather than drop its late result while the service call keeps running. --- CONTRIBUTING.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9b59299c2c..72db0b536e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -439,6 +439,13 @@ the path without a terminal. `src/handlers/gateway/invoke/index.tsx` is the mode 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 From 736196f2821f99d7cc3b3484230069f721d9b788 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 18:52:07 +0000 Subject: [PATCH 11/14] docs: recordings carry real account IDs; stop telling contributors to scrub them Earlier commits on this branch told contributors to scrub account IDs and ARNs out of fixtures and golden files, added a PR-template box for it, and made it a review finding. That contradicts the repository: 112 of the 136 committed golden files and 379 of the 496 fixtures contain real account IDs in their ARNs. - CONTRIBUTING.md: recordings carry the recording account's identifiers, and a re-record from another account rewrites every ARN. Review the diff for changes beyond those. Call uniquePerRecording inside the test, not at module load, or replay fails the whole file when the value was never recorded. - PR template: the box now asks that changes beyond account identifiers are explained by the PR. - review.md: flag unexplained changes in re-recorded files and hand-edited recordings, not the presence of account IDs. --- .github/harness/prompts/review.md | 7 ++++--- .github/pull_request_template.md | 4 ++-- CONTRIBUTING.md | 23 +++++++++++------------ 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/.github/harness/prompts/review.md b/.github/harness/prompts/review.md index acf79537aa..d345728e66 100644 --- a/.github/harness/prompts/review.md +++ b/.github/harness/prompts/review.md @@ -69,9 +69,10 @@ cheap to miss by reading a diff in isolation. 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. -- **Account-specific values in fixtures or golden files** - Account IDs, ARNs, or generated resource - names recorded from the author's own account do not reproduce elsewhere. Hand-edited or - reformatted recordings are also a finding: they must come from `RECORD=1`. +- **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 diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 0003ac1095..01d54ea2e1 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -42,8 +42,8 @@ How have you tested the change? - [ ] 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, the diff contains no account IDs, ARNs, or other - values specific to the account I recorded against +- [ ] 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/CONTRIBUTING.md b/CONTRIBUTING.md index 72db0b536e..5abee6d1bb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -556,18 +556,17 @@ stable. Golden files are excluded from Prettier (`.prettierignore`) — they are byte-for-byte recordings, not source to reformat. Record against your own AWS account. A contributor adding a flag to a command with a -fixture-backed suite re-records that suite with `RECORD=1` using their own credentials, then -scrubs the result before committing — the first point below is what makes that safe. - -Four things to know before you record one: - -- **Scrub account-specific values out of the recording.** `matchGolden` strips presigned - URLs and normalizes JSON, but nothing else. An account ID, an ARN, or a generated - resource name recorded from your own account is committed verbatim and will not - reproduce on anyone else's, so the next person to re-record sees a diff with no - behavior change behind it. Use `uniquePerRecording` (`src/testing/fixtures.tsx`) for - values that must stay stable across re-records, and keep account-derived identifiers - out of asserted output. +fixture-backed suite re-records that suite with `RECORD=1` using their own credentials. + +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, and use + `uniquePerRecording` (`src/testing/fixtures.tsx`) for values that must stay stable across + re-records — 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 From 5947d5f45bfd374f820083d490b7d7b0797ec5d9 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Fri, 30 Oct 2026 18:56:07 +0000 Subject: [PATCH 12/14] docs: repeated requests in recordings, JsonKey, and partial flag overlap - The fixture path depends only on the request, so a request sent several times (a poll) records to one file, each call overwriting the last, and replay serves the final response to every call. - Name ctx.value(JsonKey) as how a leaf chooses between human-readable and --json output; 29 handlers use it and neither standards file mentioned it. - Clarify the shared-flags rule: only flags identical across the commands go on the class; flags that differ in schema, required-ness or description stay on each command. --- AGENTS.md | 6 +++++- CONTRIBUTING.md | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3aad820447..29191bd5e8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,7 +66,9 @@ 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. A key whose value is `undefined` is dropped before hashing (the key is + 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 @@ -126,6 +128,8 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che 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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5abee6d1bb..9f8cdf6a59 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -379,7 +379,9 @@ When two or more commands take the same set of flags, put the set on a class wit `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. +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 From 1dc15813c60b40e49c332dfe8c47a5e2888e6df2 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 2 Nov 2026 15:37:18 +0000 Subject: [PATCH 13/14] docs: address review on contribution standards - Remove links and references to the constructs repository, which is not publicly readable. Refer to the public @aws/agentcore-cdk package instead, and drop the PR-template section that asked for a link into it. - Recording: maintainers re-record fixture-backed suites against the project's test account. A contributor whose change alters a recorded request covers it with a TestCoreClient command-flow test and names the suite in the PR. Never record against an account that runs production workloads or holds customer data. - Describe uniquePerRecording correctly: a new value on every recording, the saved one on every replay. It was described as stable across re-records. - Test-support files are *-test-support.ts, matching the .oxlintrc.json override; the new line said *.test-support.ts. - Fixture paths include src/core/**/__fixtures__/. - Replace the config.insights example, which is not a real field, with spec.efsAccessPoints from src/core/project/templates/export.ts. - Name RegionExplicitKey as the existing way to tell an explicit --region from a resolved default. --- .github/harness/prompts/review.md | 2 +- .github/pull_request_template.md | 6 ---- AGENTS.md | 50 +++++++++++++++++-------------- CONTRIBUTING.md | 36 ++++++++++++---------- 4 files changed, 48 insertions(+), 46 deletions(-) diff --git a/.github/harness/prompts/review.md b/.github/harness/prompts/review.md index d345728e66..f3740f9518 100644 --- a/.github/harness/prompts/review.md +++ b/.github/harness/prompts/review.md @@ -97,5 +97,5 @@ cheap to miss by reading a diff in isolation. 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, the released L3 construct, and the `@aws/agentcore-cdk` pin bump in + 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 01d54ea2e1..9231af480d 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -12,12 +12,6 @@ Closes # -## Related Changes in Other Repositories - - - ## Type of Change diff --git a/AGENTS.md b/AGENTS.md index 29191bd5e8..c2e2fa873a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -93,25 +93,27 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che 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 `--flag`" from "something resolved a default", capture that - 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. +- **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), the L3 - construct released from `aws/agentcore-l3-cdk-constructs`, then the `@aws/agentcore-cdk` pin in + `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.** `config.insights` existing as `[]` - is not the same as the feature being configured; test `(x?.length ?? 0) > 0`. +- **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. @@ -122,11 +124,12 @@ 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), `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. +- 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. @@ -147,11 +150,12 @@ Before finishing a change run `bun test`, `bun run typecheck`, `bun run lint:che ## Testing patterns -- Tests live beside code as `*.test.ts(x)`; name any test-support file `*.test-support.ts` 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. +- 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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9f8cdf6a59..f59ee7d873 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -45,23 +45,23 @@ different times: 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__/` are recordings. Refresh +- **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 span repositories +## Changes that reach a deployed resource -The CLI vends CDK projects built from [`@aws/agentcore-cdk`](https://github.com/aws/agentcore-l3-cdk-constructs), -pinned in `src/assets/cdk/package.json`. A change that has to reach a deployed resource lands in -three steps, in order: +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. The L3 construct change merges and is released in `aws/agentcore-l3-cdk-constructs`. +2. A released `@aws/agentcore-cdk` supports it. 3. The pin in `src/assets/cdk/package.json` is bumped to that release. -Link the companion PR from this one, and expect the pin bump to be a follow-up rather than part -of the same change. +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 @@ -425,8 +425,8 @@ 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 the companion L3 -construct (see [Changes that span repositories](#changes-that-span-repositories)). Add a +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 @@ -557,18 +557,22 @@ 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 against your own AWS account. A contributor adding a flag to a command with a -fixture-backed suite re-records that suite with `RECORD=1` using their own credentials. +Maintainers re-record fixture-backed suites against the project's test account. If your change +alters the request a recorded suite sends, cover the new behavior with a `TestCoreClient` +command-flow test and say in the PR which suite needs re-recording; a maintainer records it before +merge. Never record against an account that runs production workloads or holds customer data. 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, and use - `uniquePerRecording` (`src/testing/fixtures.tsx`) for values that must stay stable across - re-records — call it inside the test, not at module load, or replay fails the whole file when the - value has never been recorded. + 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 From 623becf1eb304740513dad802719687b1be143d9 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 2 Nov 2026 20:16:12 +0000 Subject: [PATCH 14/14] docs: record in your own account, with setup steps Replace the maintainer-only recording rule with the intended one: contributors record fixture-backed suites in their own AWS account, and any account works. Maintainers record against the project's own account, so a recording may be re-made later with different identifiers. Add a short setup guide for recording: credentials through the default chain, us-west-2 (every suite pins it), the permissions a suite needs, pre-existing resources pinned in FIXTURE_* constants, and the RECORD=1 command. --- CONTRIBUTING.md | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f59ee7d873..e54bc15192 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -557,10 +557,22 @@ 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. -Maintainers re-record fixture-backed suites against the project's test account. If your change -alters the request a recorded suite sends, cover the new behavior with a `TestCoreClient` -command-flow test and say in the PR which suite needs re-recording; a maintainer records it before -merge. Never record against an account that runs production workloads or holds customer data. +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: