Repository navigation
docs: close gaps in the contribution standards - #2737
Conversation
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.
agentcore-devx-automation
Bot
added
agentcore-harness-reviewing
Package TarballHow to installgh release download pr-2737-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-1.0.1.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v1.x #2737 +/- ##
==========================================
- Coverage 96.54% 96.54% -0.01%
==========================================
Files 706 706
Lines 58529 58529
==========================================
- Hits 56508 56507 -1
- Misses 2021 2022 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
agentcore-devx-automation
Bot
removed
the
agentcore-harness-reviewing
![agentcore-devx-automation[bot]](https://avatars.githubusercontent.com/u/2232217?s=60&v=4){kind=small}
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Changes requested
Verified against base v1.x. Most of the added material checks out (the verify.yml gates, release-prepare.yml:63, the e2e trigger surface, the fixtureFactories client list, the two fixture/golden failure messages, matchGolden's presigned-URL-only sanitization, fixture-key hashing, and the withRegion/RegionKey resolution order). Four claims are inaccurate or unsatisfiable as written, and because two of them also land in review.md and the PR template they will produce recurring false findings and an uncheckable box.
1. uniquePerRecording guidance is inverted — CONTRIBUTING.md:474
Use
uniquePerRecording(src/testing/fixtures.tsx) for values that must stay stable across re-records
The helper does the opposite. src/testing/fixtures.tsx:62-74: "a value that is unique for every recording but identical on every replay" — RECORD=1 regenerates it, so it is stable across replays, never across re-records. It is also not a tool for account-specific values (its use case is session ids). This also contradicts AGENTS.md:64-65 in the same tree, which already describes it correctly as "per-recording unique values".
Options: drop the sentence, or reword to "use uniquePerRecording for a value that must differ on every recording but replay identically (e.g. a session id)".
2. Wrong glob for test-support files — AGENTS.md:122
name any test-support file
*.test-support.ts
The convention is the hyphen form, *-test-support.ts: all five existing files use it (src/core/project/migrate/migrate-test-support.ts, src/core/project/legacy/fixtures-test-support.ts, src/handlers/project/add/gateway-test-support.ts, …), and that is the glob in the oxlint override (.oxlintrc.json:19), already documented 80 lines earlier at AGENTS.md:40. A file named per the new line would fall outside the lint override, i.e. the doc change actively causes the problem it is trying to prevent. Change to *-test-support.ts.
3. The one-way import rule is not true of this tree — CONTRIBUTING.md:397-402 and .github/harness/prompts/review.md:81-82
src/core/must not import fromsrc/handlers/. Core implements contracts the handlers declare; it does not know they exist.
46 non-test files under src/core/ import from src/handlers/ today — mostly handler-declared type contracts (src/core/gateway.tsx, runtime.tsx, harness.tsx, identity.tsx, eval.tsx, project/backends/cdk.ts, project/backends/types.ts, project/migrate/*), plus value imports: RESOURCE_LABELS (src/core/project/manager.tsx:32), templateManagesDependencies / resolveRuntimeTemplateProfile (manager.tsx:111-112), templateUsesModel (project/templates/runtime.ts), DROPPED_FIELDS_WARNING (project/migrate/convertSpec.ts:6), CDK_LIBRARY_OVERRIDE_ENV (project/migrate/synth.ts:5).
Note the internal tension: "Core implements contracts the handlers declare" implies Core imports those handler-declared types, which is exactly what the code does. As stated the rule is absolute, and because it is also a review.md pattern, the reviewer bot will flag ordinary type imports on most Core PRs.
Options:
- (a) State the boundary that actually holds: Core may depend on handler-declared types/contracts, but not on handler runtime, rendering, or routing code.
- (b) Narrow it to the concrete violations worth flagging (Core importing handler helper functions / presentation).
- (c) If the absolute rule is the intended end state, land it as an oxlint
no-restricted-importsrule plus the cleanup, and document it once it is enforceable.
4. The "no account IDs or ARNs" checkbox cannot be satisfied — .github/pull_request_template.md:45-46, CONTRIBUTING.md:470-476
- 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
Committed fixtures already contain hundreds of real 12-digit account IDs and ARNs under src/handlers/**/__fixtures__/ (one account ID appears 318 times). Nothing in the record path scrubs them — matchGolden sanitizes presigned URLs only, as the same bullet correctly says — and fixture keys hash the request input, so a legitimate re-record necessarily writes the recorder's account identifiers back into the files. A contributor doing the right thing must leave a required box unchecked, which trains people to check it anyway.
Options:
- (a) Reword to the finding actually being guarded against: don't commit a re-recording whose only diff is swapped account identifiers, and keep re-records scoped to the fixtures your change touches.
- (b) Add a sanitizer to the recording path first (code change, separate PR), then document the prohibition.
- (c) Scope the checkbox to new credentials/secrets, which
secrets:checkalready gates, and drop the account-ID clause.
Smaller notes (not blockers)
CONTRIBUTING.md"Fixtures and golden files undersrc/handlers/**/__fixtures__/" missessrc/core/project/legacy/__fixtures__/andsrc/core/project/migrate/__fixtures__/.AGENTS.md:94usesconfig.insightsas the example, but no such field exists in the tree; a real field or a neutralxavoids pointing readers at nothing.AGENTS.md"capture that before the middleware runs" —RegionExplicitKeyalready does exactly this (src/middleware/withRegion.tsx:19-22). Naming it makes the guidance actionable instead of inviting a reimplementation.review.md:83-85("Free functions that should be methods") is a structural-style preference sitting under an instruction to "skip style nits"; expect noise from it.
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.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
- 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.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
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.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
- 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 <ResponseType>`, the idiom used across the suite; a bare `as <ResponseType>` compiles until the literal gains a nested optional, then fails comparability with TS2352.
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
vivdalal
left a comment
vivdalal
left a comment
There was a problem hiding this comment.
Thanks for tightening this contributor guidance. I did not find any actual credentials, tokens, or customer data in the diff. I do think a few items should be addressed before this becomes public contributor guidance:
-
Public accessibility: This is a public repository, but the companion repository/PR references currently return 404 for unauthenticated readers. The references appear in the PR description,
CONTRIBUTING.md, and the PR-template comment. Could we replace them with public documentation, or omit them until the destination is publicly accessible? Otherwise the published contribution instructions contain dead links. -
Recording-account identifiers: The proposed language establishes real recording-account IDs as expected committed content and instructs contributors to use their own credentials. Could we instead require an isolated non-production account, explicitly prohibit production/customer data, and normalize or redact account-specific identifiers before commit? Account IDs are not credentials, but avoidable persistent identifiers in public fixtures are still undesirable. The PR-template checkbox should enforce that policy as well.
-
Core import rule: The PR description says
src/core/must not importsrc/handlers/, while the added documentation says Core imports contract types declared undersrc/handlers/<domain>/types.tsx. Please make the rule precise—for example, type-only contract imports are permitted, while presentation and runtime imports are prohibited. -
Narrative cleanup: The description still has a Second commit section even though the PR now contains 12 commits. Also, the checked “Any dependent changes have been merged and published” assertion should be verified or marked not applicable based on the dependency described above.
These are disclosure and documentation-consistency concerns; I did not find a secret in the current patch.
- 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.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
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.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
|
@vivdalal thanks for the review. Point by point: 1. Public accessibility. Fixed in 2. Recording-account identifiers. I'm keeping contributors recording in their own AWS account, and any account works. Maintainers record against the project's own account, so a recording can be re-made later with different IDs in it. I added a setup guide in I'm not adding redaction or a template checkbox:
A redaction step would also be a code change to the recorder. That belongs in its own PR if we want it. 3. Core import rule. Made it precise. Core may import the type contracts a handler declares in its I didn't ban every runtime import. 9 runtime imports exist today, all pure constants and helpers in the 4. Narrative cleanup. Done. I removed the stale "Second commit" section and rewrote the description to match all the commits. The "Any dependent changes have been merged and published" box now reads "not applicable", since the PR no longer depends on the constructs repo. |
Description
Brings
CONTRIBUTING.md,AGENTS.md, the PR template and the AI review prompt in line with what CI enforces and what reviewers repeatedly ask for. Documentation only; no code or behavior change. Every claim is checkable in this repository; references are in #2736.Checks. Adds
bun run auditandbun run secrets:check, both gates inverify.yml, to the required checks and the PR template.Generated files.
command.mdis regenerated and committed byrelease-prepare.yml, so feature PRs leave it alone. This correctsAGENTS.md, which told contributors to regenerate it.Recording. Contributors record fixture-backed suites in their own AWS account, and any account works; CONTRIBUTING.md has the setup steps. Documents the cases that make a command unrecordable until the harness is extended (a per-run value in the request, a streaming response), that
undefinedrequest fields do not change the fixture hash, that repeated identical requests share one fixture, the two distinct missing-fixture and missing-golden failures, and whatuniquePerRecordingis for.Architecture. Core may import the type contracts a handler declares in its
types.tsx; it must not import presentation (screens, components, routers, user-facing text). Also: the per-change file inventory includingTestCoreClient, route registration inRoot.tsx, TUI scope (supportedTuiCommandsis per router; a branch's TUI launch is its parent's decision),AbortSignalfor Core calls a screen can cancel, leaf deep links viarenderTuiAt,SilentCLIError,JsonKey, shared flag groups as a class, and changes that reach a deployed resource via@aws/agentcore-cdk.Corrections to existing text. Re-exporting
screen.tsxfromindex.tsxis not the convention (Root.tsximports screens directly);createRootHandlerhas noglobalConfigoption.Review prompt. Adds the findings reviewers raise repeatedly so the bot catches them before a human round.
Related Issue
Closes #2736
Documentation PR
This is the documentation change.
Type of Change
Testing
bun run format:checkbun test,bun run test:e2e,bun run typecheck,bun run lint:check,bun run audit,bun run buildbun run secrets:check: not run locally; no credentials or secrets addedcommand.mdChecklist