docs: make the pull request template followable - #47
Conversation
Two instructions in it could not be followed as written. The title rule asked for 50 characters or fewer, and the squash subject is the title with ` (#N)` appended -- this repository allows squash and nothing else, with `squash_merge_commit_title: COMMIT_OR_PR_TITLE`. So a contributor who used the whole budget landed a longer subject on `main`. Fourteen subjects there exceed 50 and six of them are inside it before the number, which is the failure this describes. `gitlint` holds the bound on every local commit and never sees a merged subject, so the template is the only place it can be kept. The answer-key note points at both tier readmes for how to produce one. Only the CLI tier's says: generate by running the tool, never by hand. The transform tier's readme said nothing about it, and that is the half with no regeneration command at all, so it is where hand-writing a key is likeliest and where the rule was missing. The rule now sits in that readme too, which makes the pointer true rather than narrowing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoClarify PR title limits and corpus answer-key generation
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
=======================================
Coverage 87.40% 87.40%
=======================================
Files 3 3
Lines 691 691
=======================================
Hits 604 604
Misses 87 87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟢 Approval recommended
The changes are documentation-only, consistent with the stated rationale, and do not introduce any correctness or maintainability issues.
Pull request overview
Updates contributor-facing documentation to make the PR template’s guidance accurate and actionable, especially around title length constraints under squash-merge and how to produce corpus answer keys without “hand-authoring” expected output.
Changes:
- Clarifies in the PR template that GitHub appends
(#N)to the squash subject, so the 50-character bound must account for that suffix. - Adds explicit guidance to
corpus/README.mdto generateexpected.mdby running the tool and reviewing diffs (and not by writing/editing expected output by hand).
File summaries
| File | Description |
|---|---|
corpus/README.md |
Documents the “generate expected output by running the tool” rule in the transform-tier corpus docs. |
.github/pull_request_template.md |
Adjusts title-length guidance to reflect squash-merge title suffixing and where the 50-char bound must be enforced. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
[Review-Convergence] Round 1: active
The opening preflight found the head two commits behind main, so this round stopped before thread triage. The catch-up brings in changes to |
The template now says the squash subject is the title with the number appended, and asks for 50 characters or fewer once it is on. This file still asked for 50 on the title alone and said to write pull request titles the same way as commit subjects, so a contributor following it lands an over-limit subject -- the failure the template change exists to remove. The code span drops the leading space the template uses, because markdownlint MD038 rejects it outside an HTML comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 7649558 |
|
[Review-Convergence] Round 3: active
Both Qodo threads are dispositioned and resolved. The title-limit finding was a divergence this pull request opened — the template gained the post-number bound and Copilot's earlier clean signal invalidated rather than propagating: the fix made |
There was a problem hiding this comment.
🟡 Changes recommended
There are small but concrete documentation issues in the changed lines (an inconsistent (#N) vs (#N) append string, and invalid -- content inside an HTML comment) that should be corrected to keep the template reliably followable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
The suffix is a space and the number, so a budget computed against the number alone is one character too generous: a 47-character title on a single-digit pull request lands a 51-character subject. Saying it in words keeps the space visible without putting it inside a code span, which markdownlint MD038 rejects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A double hyphen is not allowed in HTML comment text, and this was the only comment body in the tree carrying one. The colon is what stood here before and reads the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/agentic_review |
|
[Review-Convergence] Round 4: active
The second pass found two defects, both introduced here, and both are fixed. Qodo and Copilot independently caught the same off-by-one in the round-3 wording: the squash suffix is a space and the number, so naming the number alone left the budget one character too generous — a 47-character title on a single-digit pull request lands a 51-character subject. Fixed in d8ccfb9. Copilot separately flagged the double hyphen in the template's HTML comment, which HTML disallows in comment text and which was the only such comment body in the tree; a77367c restores the colon All three threads are resolved and both reviewers are re-requested. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are documentation-only, consistent across the referenced docs, and verified against existing regeneration tooling references in the repository.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Code review by qodo was updated up to the latest commit a77367c |
|
[Review-Convergence] Round 5: converged-merge-blocked
Bot-review convergence is reached on this head. Copilot returned approval-recommended with no threads, and Qodo posted a real-review marker naming this commit with no unresolved threads and no active summary findings, so quorum is met from both families. Base is an ancestor of the head and all five review threads are resolved. Five findings were raised across the cycle. Four were fixed: the title-limit divergence this pull request opened, the off-by-one that the first fix introduced by naming the number without its leading space, and the double hyphen in the template's HTML comment. One is an evidenced wontfix — the transform-tier answer-key contradiction predates this change and is settled in #65.
|
What changes, and why
The title rule could not be met. The template asks for "imperative, lowercase, 50 characters or fewer", and the squash subject is the title with
(#N)appended:So a contributor who used the whole budget lands a subject over the bound. On
main, 14 subjects exceed 50 characters and 6 of those are within 50 before the appended number — the failure this describes, already in the history..gitlintsetsline-length=50and runs atcommit-msg, so it holds the bound on every local commit and never sees a merged subject; the template is the only place it can be kept. It now says the number gets appended, and why the bound is manual here.The answer-key pointer sent people somewhere the rule was not. The template says "corpus/README.md and corpus/cli/README.md say how." Only the CLI tier's does —
corpus/cli/README.md:75, "Generateexpected/andstdout.txtby running the tool rather than by writing them out."corpus/README.md's "Adding a case" section said nothing about it.That is backwards from where the rule is needed. The transform tier has no regeneration command —
REGENERATE_CLI_CORPUSbelongs to the CLI tier — so it is the half where hand-writing a key is easiest and least likely to be caught, andCONTRIBUTING.md:35already says so: "There is no such path for the transform tier, so produce those two files by running the tool and reading the diff."The rule now sits in
corpus/README.mdas well, which makes the template's pointer true rather than narrowing it to the one tier that already had it.The same rule lived in a third place.
CONTRIBUTING.mdsaid pull request titles become the squash subject and to write them the same way as commit subjects, which is 50 characters or fewer — the bound the template now states after the appended number. Following the contributing guide therefore still produced an over-limit subject, so it now names the space and the number too, and the three contributor-facing places agree. The template keeps the code-span form;CONTRIBUTING.mdstates the space in prose, because markdownlint MD038 rejects a leading space inside a code span anywhere but an HTML comment. That comment also keeps its colon rather than a double hyphen, which HTML does not allow in comment text.Corpus
The corpus is the specification, and both implementations answer to it. Tick what applies.
corpus/pins the new behavior. The case was written first and failed first.Documentation of how to write a case. No case, key or harness changes.
Checks
make checkpasses, ormake testdoes and this touches no Rust.make tidy, plusunwrap-markdown-prose-py --fail-on-changeover the three changed files —corpus/README.mdandCONTRIBUTING.mdare both in scope for this repository's own hook, since.unwrapignorenames the two corpus tiers rather than the whole tree.Closes #63 (review-convergence bulletin)