PR write-ups: the short summary names no code, so a busy reader can follow it - #434
Merged
EdbertChan merged 7 commits intoSep 12, 2026
Conversation
…ames Every pull request comes with a short written summary. Before review, a checker reads it and blocks text that is too hard to read. The problem: summaries full of code names still passed. The cause: the reading-grade scorer swapped every backticked name for "X", so each name scored as an easy word. The fix: summary-reading-grade.mjs gains findCodeNames(), which reads ## Summary and ## Review Claim and returns every backticked span, snake_case or camelCase word, file path, and word equal to a changed file or folder name. It has three outcomes: hard, clean, and unchecked (a section is missing). validate-pr-body.mjs passes --changed-files-file into it, fails with one error that lists each name, and prints a line when the check is unchecked. Later sections (Test Plan, Revert Plan, Architecture) are not read, so they may still hold names and output. SKILL.md's Summary guidance now says the reader is a busy director who has never seen the code, gives the what / problem / cause / fix shape, and adds a before-and-after worked example. Two old test fixtures used backticks in their Summary; they are updated to match the new rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he PR checker fail on code names in Summary and Review Claim, and rewrite the draft-pr writing rule for a reader who has never seen the code. Review claim: A PR body whose Summary or Review Claim contains a code name now fails the PR body checker, and the error names each one; the draft-pr skill tells writers to teach the reader from zero. Review lane: behavior Safety invariant: A PR body that passes today still passes unless its Summary or Review Claim contains a code name; Test Plan, Revert Plan, Before and After, and Architecture sections may still hold code names and output. Effectiveness measurement: PR 416's current body (Summary contains stop_hook_active, diu-stop, prove-it-ship-gate) fails the checker, and its plain rewrite (no code names in Summary or Review Claim) passes; both are pinned as test fixtures. Slice rationale: The checker, its test, and the skill text that explains the checker are one claim; shipping them apart would leave a skill rule nothing enforces, or a check the skill never explains. Architectural effect: summary-reading-grade.mjs gains a code-name check over Summary and Review Claim; the PR body checker script reports its findings as errors; the drafter-core npm package is untouched. Goal: A busy reader can read the Summary and Review Claim of any PR and understand what changed without knowing the code. Motivation: The user asked that all PRs be written in plain English for a director in a rush who does not know the architecture. The current checker passed PR 416's jargon-heavy Summary because engine/skills/draft-pr/scripts/summary-reading-grade.mjs line 18 replaces every backticked name with "X". Alternative considerations: Lowering the reading-grade limit was ruled out because code names already score as easy words, so no grade limit catches them. Renaming the schema headings (Review Claim, Safety Invariant) was ruled out for this slice because the heading set lives in the drafter-core npm package, a separate project. A word-list jargon detector was ruled out as noisier than a code-name check. Implementation details: In summary-reading-grade.mjs add an exported function that reads the Summary and Review Claim sections and returns every code name in them. A code name is any backticked span, any snake_case or camelCase word, any file path (contains a slash or ends in a file extension), and any word equal to a folder name or file base name from the changed files the checker receives through --changed-files-file. Keep three outcomes (hard, clean, unchecked); a missing section is unchecked, never clean. The PR body checker script pushes one error naming every code name found and the fix. In the draft-pr SKILL.md, the Summary guidance (current lines 86-94) becomes: the reader is a busy director who has never seen the code; the first paragraph says what the part is and what it does for a person; then the problem, the cause, and the fix, one short paragraph each; no code names in Summary or Review Claim; names and output go in later sections. PR 416's before and after Summary is the worked example. New cases go in the draft-pr test file. Non-goals: No change to the drafter-core npm package, to section headings, to the reading-grade limits, to diff-atomicity linting, or to the Invoker repo's own make-pr skill. Layer: domain Feature state: active Files: engine/skills/draft-pr/scripts/summary-reading-grade.mjs, engine/skills/draft-pr/scripts/validate-pr-body.mjs, engine/skills/draft-pr/SKILL.md, engine/skills/draft-pr/tests/test_draft_pr_scripts.py Change types: - engine/skills/draft-pr/scripts/summary-reading-grade.mjs: modify - engine/skills/draft-pr/scripts/validate-pr-body.mjs: modify - engine/skills/draft-pr/SKILL.md: modify - engine/skills/draft-pr/tests/test_draft_pr_scripts.py: modify Acceptance criteria: - A Summary containing `stop_hook_active`, diu-stop, and prove-it-ship-gate (with those two directory names passed via --changed-files-file) makes validate-pr-body.mjs exit 1 and print all three names. - A Summary written in everyday words, including ordinary hyphenated words like brand-new and word-count, exits 0. - A body with no Summary prints that the code-name check is unchecked and does not print a clean result. - Code names inside Test Plan, Revert Plan, Before and After, and Architecture do not cause a failure. - python3 engine/skills/draft-pr/tests/test_draft_pr_scripts.py exits 0. Exit code: 0
…ack PR preflight on the finished change. Review claim: The make-pr preflight passes on the change. Review lane: proof Safety invariant: Proof-only; adds no product behavior. Effectiveness measurement: Preflight prints "ok preflight passed" and exits 0. Slice rationale: One proof step for the repo's own publication gate. Architectural effect: None; verification only. Goal: Prove the change meets catstack's own PR gates. Motivation: The catstack make-pr overlay requires preflight to pass before publishing. Alternative considerations: Running each gate by hand was ruled out; preflight runs the gates for the touched paths. Implementation details: Run make-pr preflight against origin/main. Non-goals: No product edits here; proof only. Layer: app_regression Feature state: active Exit code: 0
… test file on the finished change. Review claim: The draft-pr test file passes on the change. Review lane: proof Safety invariant: Proof-only; adds no product behavior. Effectiveness measurement: The test file includes the PR 416 before and after fixtures and exits 0. Slice rationale: One proof step for the one review claim. Architectural effect: None; verification only. Goal: Prove the checker change passes its own tests. Motivation: A changed skill must pass its own tests before it ships. Alternative considerations: Running the full 45-suite run_all_tests.sh was ruled out as slower than needed for a change confined to draft-pr. Implementation details: Install npm dev dependencies, then run the draft-pr test file. Non-goals: No product edits here; proof only. Layer: app_regression Feature state: active Exit code: 0
…t/g0.t0.a-a6dff3462-f563b3f8
…k that no temporary handoff files are left on the branch before the PR merge gate. Review claim: The branch carries no leftover handoff files. Review lane: proof Safety invariant: Read-only; never deletes files or changes the index. Effectiveness measurement: scripts/scrub-handoff-artifacts.sh exits 0 on the finished branch. Slice rationale: Required terminal gate for implementation plans. Architectural effect: None; verification only. Goal: Keep temporary files out of the PR. Motivation: Invoker tasks can leave handoff files that must not ship. Alternative considerations: None; this is the standard terminal gate. Implementation details: Run scripts/scrub-handoff-artifacts.sh without --apply. Non-goals: No edits. Layer: app_regression Feature state: active Exit code: 0
…a4674c7b2-65838c37 — Read-only check that no temporary handoff files are left on the branch before the PR merge gate. Review claim: The branch carries no leftover handoff files. Review lane: proof Safety invariant: Read-only; never deletes files or changes the index. Effectiveness measurement: scripts/scrub-handoff-artifacts.sh exits 0 on the finished branch. Slice rationale: Required terminal gate for implementation plans. Architectural effect: None; verification only. Goal: Keep temporary files out of the PR. Motivation: Invoker tasks can leave handoff files that must not ship. Alternative considerations: None; this is the standard terminal gate. Implementation details: Run scripts/scrub-handoff-artifacts.sh without --apply. Non-goals: No edits. Layer: app_regression Feature state: active
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c44f717f-1c9a-4f49-be0c-3ecb45acf6fc) |
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The PR checker helps reviewers judge proposed work by checking how its write-up reads.
A short write-up could pass while still naming parts of the code that a busy reader would not know.
The reading score treated names inside backticks as one short word, so it missed the problem.
The checker now rejects code names in Summary and Review Claim. The writing guide asks authors to explain the part, then the problem, cause, and fix.
Review Claim
Reject code names in Summary and Review Claim, name each match in the error, and guide authors to explain what the part does in everyday words.
Review Lane
behavior
Review Unit
engine-runtime
Safety Invariant
Previously valid bodies remain valid unless Summary or Review Claim contains a code name or code block. Technical names remain allowed in later sections; existing schema and reading-grade limits stay intact.
Slice Rationale
The checker, regression tests, and writing rule enforce one plain-English requirement. They ship together because changed skills must update their own tests in the same PR.
Assumptions: One behavior slice in engine-runtime, using the safety invariant supplied by the workflow.
Non-goals
No changes to drafter-core, section headings, reading-grade limits, diff-atomicity rules, or Invoker's make-pr skill.
Architecture
Before
The validator checked the schema and Summary reading score. The scoring helper replaced backticked names with “X”; no separate check rejected code names.
After
The validator also passes the body and changed-file list to
findCodeNames. Matches become validation errors; missing sections report an unchecked result. Without a changed-file list, it warns that file and folder names were not compared.Test Plan
Test Plan
npm ci— exit 0.python3 engine/skills/draft-pr/tests/test_draft_pr_scripts.py—Ran 13 tests;OK, exit 0.python3 engine/skills/make-pr/scripts/preflight.py --base origin/main— exit 0. Output excerpts:unit engine-runtime: 4 file(s);ok skill test coverage;ok skills three-harness check;ok ecosystem boundaries;ok preflight passed.python3 scripts/check_no_new_comments.py --base origin/main—ok no new comments, exit 0.bash scripts/scrub-handoff-artifacts.sh—scrub-handoff-artifacts-ok, exit 0.The PR 416 regression fixture rejects
stop_hook_active,diu-stop, andprove-it-ship-gatewith exit 1 and names all three. Its plain rewrite passes with exit 0. Tests also cover ordinary hyphenated words, missing Summary reporting, and technical names in later sections.Revert Plan
Revert Plan
git revert 31ec51a.Note
Low Risk
Changes are limited to PR authoring guidance and the draft-pr validator; no runtime product or auth paths.
Overview
PR body validation now fails when Summary or Review Claim use code-facing language—backticks, fenced blocks, snake_case, camelCase, paths, or names of changed files/folders—while reading-grade limits stay the same. Errors list each hit and tell authors to use plain language up top and put identifiers in later sections.
The draft-pr skill documents the rule with a before/after Summary example and the same constraint on Review Claim.
validate-pr-body.mjsruns the new check alongside the existing schema and readability scoring; without--changed-files-file, it still runs but warns that changed-path matching was skipped.Regression tests cover the hook-name fixture, plain rewrites, hyphenated English, missing sections, and technical names allowed in Architecture/Test Plan/Revert Plan.
Reviewed by Cursor Bugbot for commit 47bec8f. Bugbot is set up for automated code reviews on this repo. Configure here.