Report the headings the pull request template does not define - #206
Conversation
The check read only the ten `## ` headings it requires. A heading a body invented, at that level or below, was therefore never looked at: `sections` cuts out the required headings alone, so the text under an invented one was measured as part of the section above it, and whatever its title promised was never checked. Nothing said so, and pull request 178 shipped two such sub-sections inside its testing manual. Every line the check reads as a heading is now compared against the template, and one the template does not define is reported with the escape a contributor needs: a line not meant as a heading belongs in a fenced code block. Three smaller corrections come with it, each found by provoking the failure: - The scan reads the copy that keeps the shape of code between backticks rather than the one that blanks it. Blanking leaves a space, and a space after a hash is what makes a hash a heading, so `#`x`` would have been reported as a heading nobody wrote. The line itself is read out of the original at the same place, so a message names what the author typed. - `HEADING`, `HEADING_LINE` and the new pattern now share one line-reading rule. Java breaks a line at a Unicode separator where Markdown keeps one, which let the three disagree about where a line stops. - A fence closes on the spaces and tabs Markdown allows and on nothing else. A Unicode space left the block open to a reader while closing it here, and every line below was then read as markup. AGENTS.md states the rule, what the check reads as a heading, and the three shapes it does not find: a heading underlined with equals signs, one indented into a quotation or a list, and a hash line inside a raw HTML block.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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 |
github-actions
Bot
added
docs
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@MarkusPaulsen The undefined-heading scan correctly reuses the checker's ATX-heading definition while preserving source offsets and masking comments, fences, and inline code. The shared LF-only regex mode and stricter fence terminator align with the checker's normalized Markdown model; I found no blocking issue in the current patch. Several non-required CI jobs were still in progress in the captured snapshot.
Section 4 already asked for an expected result per step and for it to be observable somewhere. That was not enough to stop a manual made entirely of commands that exit zero, which is what pull request 178 shipped: five of its seven steps were a build or a check passing, and a reviewer who followed it read none of the 96 pages the pull request adds. An exit code says the command ran. It does not say that what it produced is right. The section now asks for a step where the thing itself is in front of the reviewer, and says what to do where the build cannot see the defect: a site builds cleanly while a box renders as plain text, a policy is read without complaint while the rule it expresses is not enforced, and a test passes while asserting nothing. Where a suite in this repository already looks at such a result, the manual runs it and says what it covers. No heading, limit or whole-section phrase changes, so the checker's own copy of those rules stays as it is.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@MarkusPaulsen The undefined-heading scan consistently applies the checker's ATX-heading rules while preserving source offsets and excluding recognized comments, fences, and inline code. The follow-up commit only strengthens the testing-manual guidance and introduces no enforcement or compatibility defect. The captured non-required CodeQL and Build jobs were still in progress.
2 participants
Summary
Two holes in the pull request template contract. A body could invent a heading of its
own and nothing said so, and a testing manual could be made entirely of commands that
exit zero without ever showing a reviewer the result.
Linked issues
No linked issues
1. Problem
The check read only the ten
##headings it requires, so a heading a body inventedwas never looked at.
sectionscuts out the required headings alone, which means thetext under an invented heading was measured as part of the section above it, and
whatever its title promised was never checked. A body could therefore carry sections
the template does not have, and pass.
That is what pull request 178 does: its testing manual holds
### Repository settingand
### Review, neither of which is a testing step, and the check passed it.Separately, section 4 asked for an observable expected result but not for the result
itself. Pull request 178 shows what that allows: five of its seven steps were a build or
a check passing, so a reviewer who followed it read none of the 96 pages it adds, and
neither the browser suite nor the 640 Markdown structure tests were ever run.
Both faults are in the template contract rather than in Ares. Nothing about the security
boundary changes either way.
2. Improvement from the user's perspective
No Improvement from the user's perspective
3. Improvement from the maintainer's perspective
A reviewer reads the sections the template defines. A body that adds its own sends them
somewhere the template puts nothing, and hides text from every rule the check applies,
since an invented heading is measured and scanned as part of the section above it. Both
are now reported, with the escape a contributor needs.
Section 4 now asks for a step where the result itself is in front of the reviewer, and
says what to do where a build cannot see the defect: a site builds cleanly while a box
renders as plain text, and a test passes while asserting nothing.
Three defects found while writing the heading rule are fixed as well. Code between
backticks could manufacture a heading, because blanking a backtick leaves a space and a
space after a hash is what makes a hash a heading. The heading patterns disagreed about
where a line stops, since Java breaks one at a Unicode separator where Markdown does not.
A fenced block closed on a Unicode space here while staying open to a reader.
4. Testing manual
Prerequisites
PATH. The check is a single file run through the source-code launcher,so there is no build step.
Steps
Not reproducible from an exercise. The check runs on a pull request body, not on
student code, so a reviewer verifies it by feeding it bodies from the repository root.
PR_BODY="$(gh pr view 178 --json body -q .body)" java .github/scripts/CheckPullRequestTemplate.java— expected: exit 1, and two
::error::lines naming### Repository settingand### Review. Onmainthe same command exits 0.PR_BODY="$(cat .github/PULL_REQUEST_TEMPLATE.md)" java .github/scripts/CheckPullRequestTemplate.java— expected: exit 1, and the same complaints as on
main: five empty sections, theunfilled list stub and the empty coverage row. No heading is reported, which confirms
the template does not fail its own new rule.
#hashtag, and run the checkagain — expected: exit 0. A hash has to be followed by a space, a tab or the end of
the line, so this is not a heading.
### Fencedand run the check —expected: exit 0. Fenced blocks, comments and code between backticks are painted over
before the scan.
## Summarya second time — expected: exit 1 with the existingduplicate-heading complaint, not a new one, which confirms the rule does not fire on a
heading the template does define.
.github/PULL_REQUEST_TEMPLATE.mdunder## 4. Testing manual, then read the manual youare reading now and the rewritten one on pull request 178 — expected: both have a step
that puts the result in front of you rather than an exit code, and 178 now runs the
browser suite and the 640 Markdown structure tests it previously skipped.
Expected result
Per step, above.
Negative case (what must still be rejected)
A body that passes today must still pass. Feeding the check the bodies of the last 24
human-authored pull requests in this repository leaves 16 untouched and reports 8, of
which exactly one is still open: pull request 178, whose body is corrected alongside
this. The other seven are merged and are never checked again.
The check must also never report a heading it has painted over, since that would
block a body for text nobody wrote. Verified against code between backticks in both
orders, a fenced block, a comment, four-space indented code, seven hashes, a Unicode
line separator inside a line, and a Unicode space after a closing fence.
Modes exercised
No mode-specific behaviour changed.
5. Test case coverage regarding this PR
No production Java code changed
Breaking changes and migration
Nothing an instructor consumes changes: not the public API, the policy format, the
generated security tests or the minimum JDK.
For contributors, a body that invents a heading now fails a required check that
accepted it before. Of the open pull requests only 178 is affected, and its body is
being corrected in the same effort. The fix is always an edit to the description, and
the check re-runs when the description is edited.
The section 4 wording is guidance, not something the check enforces, so no existing body
fails because of it.
The check does not recognise raw HTML blocks, so a line starting with a hash inside
<pre>or<details>is reported as a heading. Putting it in a fenced code block isthe escape, and the message says so. This is written down in AGENTS.md.
Checklist
documentation/,README.md, Javadoc) was updated where the change is user-facing.Review progress