Repository navigation
docs(github): expand the pull request template into review gates - #135
Conversation
The current template asks for a description, a type, and testing notes, but states no bar a contribution has to clear, so incomplete PRs reach review and the same gaps are raised by hand every time. Add the gates that were already being applied in review, and make them explicit: correct repository layout, a named domain SME sign-off from someone other than the author, eval evidence, no internal-only content, a complete README, a CHANGELOG entry with a version bump, and a named owner. Break the testing section out into the three eval types and require both positive and negative eval cases. Add per-entry-type checklists for skills, MCP servers and custom agents, and a security section covering read-only defaults, customer data residency and confirmation on destructive operations. Everything in the previous template is kept, including the reviewer note on exemptions.json and the enforce-evals label, and the Apache 2.0 confirmation.
udid-aws
left a comment
udid-aws
left a comment
There was a problem hiding this comment.
Important additions to the PR template, thanks. Approved, with a few non-blocking notes to consider for a future change:
best-practices run across multiple iterations, with consistent results.
I suggest to mention that the minimum iterations is 3 (that's the default with the skill eval tool)
functional run with and without the change, so the difference in the agent's behavior is visible.
I suggest to mention that it needs to run across multiple iterations, 3 at minimum (that's the default with the skill eval tool)
evals/evals.json includes positive cases (should trigger) and at least one negative case (should not trigger on unrelated input).
I suggest to rephrase to something like this: "evals/evals.json includes positive cases (should trigger) and at least one negative case (should not trigger on unrelated input). Each positive case evaluates real functional scenario that involves interaction with real AWS APIs and resources, and not instructional question."
Note the additional requirement - I think it's important to make sure that the functional evals test scenarios with real AWS resources, as sometimes they tend to be instructional, which isn't sufficient
Description
The current template asks for a description, a type and testing notes, but states no bar a contribution has to clear. The result is that incomplete PRs reach review and the same gaps get raised by hand every time. This expands it into explicit pre-review gates, adds per-entry-type checklists for skills, MCP servers and custom agents, and adds a security section. Everything in the previous template is kept.
Type of change
Required before review
Most of these gates describe tool contributions and do not apply to a change that only edits the template. Marking them honestly rather than checking them:
Testing
No eval types apply — this change adds no skill, agent or MCP server.
What I did verify:
.github/PULL_REQUEST_TEMPLATE.md.exemptions.jsonand theenforce-evalslabel (verbatim), and the Apache 2.0 confirmation.Security and safety
License confirmation
Note for maintainers — one real decision here
This is not purely editorial. The gate that changes behaviour is:
That codifies something currently applied inconsistently, and two PRs from this week show why it is worth writing down:
The hold language at the top is also new — a PR with unchecked required items is placed on hold rather than reviewed. That is a real change in how contributors will experience this repo, and it is worth agreeing to explicitly rather than inheriting it from a template edit.
Two smaller things to sanity-check before merging:
CONTRIBUTING.mddoes not state a budget. Either cite the number or soften the wording.skills/.gitignoreand the documented zip command, so it is accurate today. Worth keeping aligned if feat(ci): validate skill frontmatter on pull requests (1/4) #126–feat(ci): re-run the skill check on main, build the docs on PRs, and add code owners (4/4) #129 land, since that stack enforces the same rule in CI.