chore: add measurable impact verification to PR and issue templates#34
chore: add measurable impact verification to PR and issue templates#34venikman wants to merge 4 commits into
Conversation
Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
Original prompt from Stanislau
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughAdded a required "impact" textarea field to three GitHub issue templates (bug_report, feature_request, task) and added an "Impact verification" section to the pull request template; each asks for at least one measurable metric with before/after values and reproducible measurement instructions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mandatory 'Impact verification' section across the repository's issue and pull request templates to ensure all changes include measurable verification criteria. The review feedback identifies several opportunities to improve the accuracy of the provided examples, including updating tool counts and file references to match the current codebase, and enhancing the portability of a grep command example.
| label: Measurable impact | ||
| description: | | ||
| How will we know this feature works? List at least one metric with a before/after target and a command or method to measure it. | ||
| Example: "MCP tool count: before 8 → after 10. Measure: count tools in `src/mcp/tools.ts`" |
There was a problem hiding this comment.
The current count of MCP tools in src/mcp/tools.ts is 9. Updating the example from before 8 to before 9 ensures the template reflects the actual state of the codebase.
Example: "MCP tool count: before 9 → after 11. Measure: count tools in `src/mcp/tools.ts`"There was a problem hiding this comment.
Correct — there are 10 createTool calls currently. Updated the example to before 10 → after 12 with a concrete grep -c createTool src/mcp/tools.ts command.
| label: Measurable impact | ||
| description: | | ||
| How will we verify the impact of this task? List at least one metric with a before/after target and a reproducible measurement command. | ||
| Example: "LOC in compiler.ts: before 1509 → after ≤300. Measure: `wc -l src/runtime/compiler.ts`" |
There was a problem hiding this comment.
The example LOC for compiler.ts (1509 lines) is inaccurate as the file is currently only 145 lines long. query-engine.ts is the file that matches the ~1600 line count. Updating the example to use query-engine.ts makes it more realistic and less confusing for contributors.
Example: "LOC in query-engine.ts: before 1615 → after ≤1200. Measure: `wc -l src/runtime/query-engine.ts`"There was a problem hiding this comment.
Right — compiler.ts is now 144 lines after the split. Updated example to reference query-engine.ts (1614 lines) which is the actual large file.
|
|
||
| | Metric | Before | After | How to measure | | ||
| |--------|--------|-------|----------------| | ||
| | <!-- e.g. hard-coded node IDs in query-engine.ts --> | <!-- e.g. 12 --> | <!-- e.g. 0 --> | <!-- e.g. `grep -c 'C\.\|A\.' src/runtime/query-engine.ts` --> | |
There was a problem hiding this comment.
The grep command in the example uses \| for alternation, which is not portable across all grep implementations (e.g., it fails on BSD grep used in macOS). Using grep -E (Extended Regular Expressions) is the standard, portable way to perform alternation.
| | <!-- e.g. hard-coded node IDs in query-engine.ts --> | <!-- e.g. 12 --> | <!-- e.g. 0 --> | <!-- e.g. `grep -c 'C\.\|A\.' src/runtime/query-engine.ts` --> | | |
| | <!-- e.g. hard-coded node IDs in query-engine.ts --> | <!-- e.g. 12 --> | <!-- e.g. 0 --> | <!-- e.g. `grep -Ec 'C\.|A\.' src/runtime/query-engine.ts` --> | |
There was a problem hiding this comment.
Good catch — switched to grep -Ec for portable alternation. Fixed in the next commit.
Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
There was a problem hiding this comment.
Pull request overview
Updates GitHub PR/issue templates to require authors to specify measurable impact and how reviewers can reproduce/verify it, strengthening reviewability and outcome verification across the repo.
Changes:
- Added an Impact verification section to the PR template with a metric before/after table and measurement examples.
- Added required Measurable impact fields to the Feature Request and Task issue forms.
- Added required Measurable impact of the fix field to the Bug Report issue form.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/pull_request_template.md | Adds an “Impact verification” section and examples to make PR impact measurable/reproducible. |
| .github/ISSUE_TEMPLATE/feature_request.yml | Adds a required “Measurable impact” textarea with before/after target format. |
| .github/ISSUE_TEMPLATE/task.yml | Adds a required “Measurable impact” textarea with before/after target format. |
| .github/ISSUE_TEMPLATE/bug_report.yml | Adds a required “Measurable impact of the fix” textarea with broken/fixed format. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| | Metric | Before | After | How to measure | | ||
| |--------|--------|-------|----------------| | ||
| | <!-- e.g. hard-coded node IDs in query-engine.ts --> | <!-- e.g. 12 --> | <!-- e.g. 0 --> | <!-- e.g. `grep -Ec 'C\.|A\.' src/runtime/query-engine.ts` --> | |
There was a problem hiding this comment.
The table includes a pre-populated (but visually empty) row whose contents are entirely HTML comments. This can make it easy to submit the template without actually providing a measurable metric, despite the instruction to "Fill in at least one row." Consider removing the placeholder row entirely (or moving the example row into the surrounding HTML comment) so the author has to add a real row.
| | <!-- e.g. hard-coded node IDs in query-engine.ts --> | <!-- e.g. 12 --> | <!-- e.g. 0 --> | <!-- e.g. `grep -Ec 'C\.|A\.' src/runtime/query-engine.ts` --> | |
There was a problem hiding this comment.
Intentional — the HTML-comment placeholder row renders as a visually empty row with the table headers visible, which shows authors the expected column structure. Removing it would leave just headers with no hint of what a filled row looks like. The surrounding comment block already has richer examples. Keeping it as-is.
|
|
||
| | Metric | Before | After | How to measure | | ||
| |--------|--------|-------|----------------| | ||
| | <!-- e.g. hard-coded node IDs in query-engine.ts --> | <!-- e.g. 12 --> | <!-- e.g. 0 --> | <!-- e.g. `grep -Ec 'C\.|A\.' src/runtime/query-engine.ts` --> | |
There was a problem hiding this comment.
The example measurement command uses grep -c 'C\.\|A\.' ... which relies on BRE alternation via \| and may not work consistently across grep implementations (notably BSD/macOS). Prefer an explicitly portable form (e.g., use grep -E/egrep or rg) in templates so reviewers can reproduce measurements on common dev environments.
| | <!-- e.g. hard-coded node IDs in query-engine.ts --> | <!-- e.g. 12 --> | <!-- e.g. 0 --> | <!-- e.g. `grep -Ec 'C\.|A\.' src/runtime/query-engine.ts` --> | | |
| | <!-- e.g. hard-coded node IDs in query-engine.ts --> | <!-- e.g. 12 --> | <!-- e.g. 0 --> | <!-- e.g. `grep -E -c 'C\.|A\.' src/runtime/query-engine.ts` --> | |
There was a problem hiding this comment.
Already addressed — the previous commit switched to grep -Ec which uses ERE explicitly. The current code on the branch already reads grep -Ec 'C\.|A\.'.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
…C) instead of compiler.ts (144 LOC) FPF-spec revalidation found the compiler.ts example misleading since it's already 144 LOC after prior splits. query-engine.ts (1614 LOC) is the actual refactor target per issue #2. Grounded in: E.19 (Pattern Quality Gates — reproducible measurement examples) Co-Authored-By: Stanislau <nedbailov375426@gmail.com>
Code ReviewOverviewAdds an "Impact verification" section to Strengths
IssuesDuplicate of #35This PR and #35 (
Recommendation: close this PR and land #35. Keeping both open will produce conflicting merges. Template friction for trivial PRsRequiring a measurable-impact table on every PR adds cost for typo fixes, docs-only changes, and mechanical renames. Consider a "trivial" opt-out row (e.g. PR template is a markdown file, not a yaml formUnlike the issue forms, the PR template can't enforce
|
|
Thanks for the thorough review. Agreed on all points:
Closing in favor of #35. |
|
Closing in favor of #35 per code review feedback. |
What
Add a required "Impact verification" section to the PR template and a required "Measurable impact" field to all three issue templates (feature request, task, bug report).
Why
Every PR and issue should declare its measurable impact upfront so reviewers can independently verify the change's effect with a reproducible command. This closes the gap between "tests pass" and "this PR actually achieved what it claims."
Type
chore— maintenance (deps, CI, cleanup)Changes
## Impact verificationsection with a| Metric | Before | After | How to measure |table and examples of measurable metrics (LOC, file count, hard-coded IDs, test count, build time, etc.)Measurable impacttextarea with before/after target formatMeasurable impacttextarea with before/after target formatMeasurable impact of the fixtextarea with before/after broken/fixed formatquery-engine.ts(1614 LOC) instead ofcompiler.ts(144 LOC) after FPF-spec revalidationImpact verification
grep -c 'Impact verification' .github/pull_request_template.mdgrep -c 'Measurable impact' .github/ISSUE_TEMPLATE/*.ymlFPF-spec revalidation
Validated all templates and 7 open issues against FPF-spec.md via
query_fpf_spec(local vectorless runtime, sha256:15a77f8e…ecd879).Key FPF alignment: E.19 (Pattern Quality Gates / PQG) mandates profile-based review with reproducible checks — the
| Metric | Before | After | How to measure |table maps directly to this. All codebase metrics verified against actual values (query-engine.ts=1614 LOC, 14 runtime modules, 10 createTool calls, 6 hard-coded IDs, 5 test files).Validation
bun run lintpasses locallybun run checkpasses locallybun run testpasses locallyBoundary check
FPF-spec.mdonly — no additional corpora addedsrc/mcp/tool-contracts.tsAgent metadata
Requested by: @venikman