Skip to content

Contribution standards drift from what CI enforces and what reviewers ask for #2736

Open

Description

@jariy17

CONTRIBUTING.md, AGENTS.md, the PR template and the AI review prompt are the four files a new contributor reads before their first PR. Several things they need are missing, and one is wrong. Each item below is checkable in this repo.

Wrong

  • AGENTS.md tells contributors to regenerate command.md after changing a command name, flag or description. The release workflow already does this — .github/workflows/release-prepare.yml runs bun scripts/generate-command-reference.mjs and commits the result in the release PR. Regenerating on a feature branch also picks up every command change merged since the last release, so the PR carries a large unrelated diff that gets asked back out.

Missing from the required checks

  • bun run audit and bun run secrets:check are gates in .github/workflows/verify.yml, but neither CONTRIBUTING.md's check list nor the PR template mentions them. A contributor who runs the five documented commands still fails CI.

Missing: the golden-file / record-replay pattern

CONTRIBUTING.md explains the pattern and AGENTS.md gives the recipe, but four things that decide whether a recorded test reproduces on anyone else's machine are undocumented:

  1. matchGolden strips presigned URLs and normalises JSON (src/testing/fixtures.tsx), and nothing else, so recordings carry the recording account's IDs in their ARNs (most committed fixtures and goldens do). A re-record from another account rewrites every ARN, and nothing tells a contributor that this churn is expected.
  2. Missing fixture … and Missing golden file … have different causes and different fixes; only the first is documented.
  3. A fresh CoreClient per run is required because it carries per-run caches. This is stated only in a code comment in src/handlers/harness/harness.test.tsx.
  4. fixtureFactories returns one factory per AWS client the CLI talks to. A handler reaching a service not in that list cannot replay until the factory is added.

Missing: cross-repository and architectural rules

  • A field that must reach a deployed resource needs the CloudFormation property, then the released L3 construct from aws/agentcore-l3-cdk-constructs, then the @aws/agentcore-cdk pin bump in src/assets/cdk/package.json. Nothing says this, and the PR template has nowhere to link the companion PR.
  • src/core/ must not import from src/handlers/. The dependency-inversion section implies it; it is not stated.
  • The e2e suite is not a PR gate — .github/workflows/e2e-test.yml runs only on workflow_call / workflow_dispatch behind an authorization check — so a contributor has to run it locally. The PR template's e2e box reads as though CI covers it.

Missing: things reviewers ask for repeatedly

The AI review prompt lists one pattern. Recurring review findings that it could catch first include comments that restate the code, tests that assert the implementation rather than behaviour, a new flag whose absent case is untested, dead guards on middleware-resolved context values, and an error hint added to a screen but missing from the headless path.

A PR follows.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions