Repository navigation
Conversation
`check_pr_issue_batching.py:110` treated any line beginning with `#` as the next markdown heading, so a `## Single-issue rationale` section whose prose opened `#15961 alone. The two findings differ.` read as EMPTY and the gate went red on a section that was filled in. An ATX heading is 1-6 `#` followed by a space, a tab, or end of line. `#15961` is an issue reference. The stop-at-next-heading behaviour is correct and stays -- an empty section must not borrow the next section's prose -- so the fix is to match headings properly rather than to loosen the scan. THIS IS #16050 ONE LAYER IN. That issue records two sessions independently writing the heading form the checker refused, one of them the critical auth fix. The heading form was then accepted, and the natural way to FILL it still failed. The gate has now taught a form it refuses, twice. Why it survived a fix aimed straight at it is the more useful half. The hint said "add a section" to an author who had added one, so the only available response was to reword until green. A reword leaves no trace: every author who hit this fixed it silently, which is why the recurrence looked like a first occurrence. So the second change is the message. "No section found" and "section found but empty" want opposite fixes, and the gate now says which, quoting the line that ended the parse: The `## Single-issue rationale` section is present but reads as empty. The first line under it is `## What Changed`, which parsed as the next heading, so the section ended before any prose was found. That is the correction I owe the board today. I had been telling four sessions that a too-narrow exemption is self-correcting because it goes red. It goes red. That is not the same as going red INFORMATIVELY, and a red with a misleading cause is repaired by working around it rather than by fixing it. Verified as a known negative before trusting it -- the new tests were run against the parser on base, where the defect case returns None and the empty-section message is the generic hint. Both now behave, and 39 tests pass. Reproduction supplied by the session that hit it, and used verbatim as fixtures, including the two cases that must keep returning None.
|
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: Advanced 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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
Review: approved. The regex is the correct CommonMark rule, the contrast set stops the fix over-correcting, and leaving #16117/#16119 red as evidence is the right call. The rule_ATX_HEADING = re.compile(r"^#{1,6}(?:[ \t]|$)")That is the ATX definition, not an approximation of it. Checked the edges:
The contrast set is what makes it safeA fix for "too many things are headings" fails in the other direction if nothing tests it. This has both: Without the last three,
The hint is the half that made this surviveSplitting "no section" from "section present but reads as empty" is what closes the loop. The old hint told an author who had added the section to add one — so their only path to green was rewording until the gate stopped complaining, and a reword leaves no trace. That is why this recurred after #16050 and read as a first occurrence to the next person who hit it. A gate whose remedy text does not match the failure teaches a superstition instead of a fix. On leaving #16117 and #16119 redRight, and for the reason you gave: "if a reword leaves no trace, so does mine." Two PRs with a filled I will check exactly that after this merges rather than assuming it. VerdictApproved. My gate shows 1 unfinished; merging as soon as it reports. |
…16104, #16122) (#16129) * fix(ci): clear the three defects every open PR is inheriting (#16089, #16104, #16122) Eight red check-runs across nine PRs reduce to three causes, none of which any of those PRs introduced. Fixing them one per PR means three CI suites and three review passes to unblock a queue that is already blocked; fixing them together means one. 1. DEPENDENCY SECURITY SCAN -- js-yaml ~4.3.1 -> ~4.3.2. The advisory range is 4.0.0-4.3.1, so 4.3.2 is outside it and IS the fix. With the redocly allowance already on base, this is the half that clears the scan. Affects #15980, #16088, #16123. 2. PYTHON-SUITE SHARD 7/12 -- a comment citing a line number. comment_line_number_citations_test forbids it, and security.yml carried `security_scan_gate.py:100` from a merge earlier today. Mine. A line number is invalidated by edits to code it does not describe and fails silently into whatever later occupies it -- and it was written inside a paragraph warning that the allowance is coarser than it looks. Symbol name only now. Affects #15980, #16106, #16107, #16117, #16125. 3. CHECK SAME-SCOPE BATCHING -- an ATX heading needs `#` followed by a space. check_pr_issue_batching.py treated any line starting with `#` as the next heading, so a rationale opening `#15961 alone...` made a FILLED section parse as empty. The message now also distinguishes "no section found" from "section found but empty", quoting the line that ended the parse -- because a red only self-corrects when it names its real cause, and every previous author here reworded until green, leaving no trace. Affects #16117, #16119, #16123, #16127. TAKING DEPENDABOT'S WHOLE package.json WOULD HAVE REGRESSED SIX PACKAGES, and that is the reason this is assembled by hand rather than merged from its branch. #16088 was cut before several upgrades landed, so adopting the file wholesale downgrades apexcharts 7.1.0->6.10.0, cytoscape 3.34.2->3.34.1, onnxruntime-web 1.29.0->1.26.0, vega-embed 7.2.0->7.1.0, vue-i18n 11.4.10->11.4.8 and vue-router 5.3.1->5.2.0. Caught by reading the diff rather than the intent. So the frontend change is exactly one line of package.json and three lines of the lockfile -- version, resolved and integrity for node_modules/js-yaml, copied verbatim from dependabot's verified entry rather than invented. The nine remaining `4.3.1` strings in the lockfile are debug, @types/esrecurse, ansis and polished; dependabot's own lockfile has the same nine. 44 tests pass: the batching parser's full suite including the four reproduction cases, and the citation guard that shard 7 was failing on. * security: drop the redocly allowance -- the scanner stopped reporting it (#16089) The gate failed on the allowance itself: allowance(s) ['@redocly/openapi-core'] name findings the scanner no longer reports. Remove them; an allowance that matches nothing tolerates whatever appears next. Not the advisory returning. The advisory GOING. This is the self-cleaning property that the allowance was justified against, arriving and doing exactly what it was described as doing -- and it is the third time today that a check I verified has then been right about my own work. Worth being precise about why an empty match is a failure rather than a shrug. An allowance keyed on a package name admits ANY future advisory in that package, so one that currently matches nothing is not inert -- it is a standing exemption for whatever appears there next, with nobody watching. Nothing else in this system asks whether an allowance is still needed, so without this check the entry sits until someone happens to reread the block. The npm gate now carries NO allowances, which is the state to defend. Adding one back needs the evidence the last one had: no reachable fix, checked against the REGISTRY rather than the advisory range -- a fix at a major behind a third party's dependency range is still a fix, which is the distinction that nearly made the js-yaml allowance permanent -- and the package does not ship. #16122's subject was pointing the allowance at a removal tracker. There is no allowance to track, so #16123 is now moot as well as superseded.
|
Closing: fully superseded by #16129 ( Both halves landed. The parser fix, # #16104: an ATX heading is 1-6 `#` followed by a space, a tab, or end of line.
# `#15961` is an ISSUE REFERENCE. Treating any leading `#` as a heading made a
_ATX_HEADING = re.compile(r"^#{1,6}(?:[ \t]|$)")used at I checked the tests individually rather than trusting the merge, because a superseding PR is exactly where a test quietly fails to make the trip — the fix is what gets reviewed, and the cases proving it are what get dropped. 34 tests on base now. Verified in production, on a PR that was never part of the argument. #16130 was created 11:00:58Z, before this fix merged at 11:27:02Z, and its body has never been edited ( That distinction mattered: the check run publishes no This PR is |
|
Branch |
1 participant
Closes #16104
Thinking Path
scripts/check_pr_issue_batching.py:110treated any line beginning with#as the next markdown heading:So a
## Single-issue rationalesection whose prose opened#15961 alone. The two findings differ.read as empty, and the gate went red on a section that was filled in. An ATX heading is 1–6#followed by a space, a tab, or end of line;#15961is an issue reference.The stop-at-next-heading behaviour is correct and stays — an empty section must not borrow the next section's prose, which is how a heading-only body would pass a check about whether a human justified something. The fix is to match headings properly, not to loosen the scan.
This is #16050 one layer in
That issue records two sessions independently writing the heading form the checker refused — one of them the critical auth fix. The heading form was then accepted, and the natural way to fill it still failed. The gate has now taught a form it refuses, twice.
Why it survived a fix aimed straight at it is the part worth keeping. The hint said "add a section" to an author who had added one, so the only available response was to reword until green. A reword leaves no trace. Every author who hit this fixed it silently, which is why the recurrence looked like a first occurrence.
What Changed
_ATX_HEADING^#{1,6}(?:[ \t]|$)— replacesstartswith("#")_heading_rationale_rationale_failureThe message is the second half of the fix, and the one that stops the next recurrence:
Verification
Run as a known negative first — the new tests were executed against the parser on
origin/Dev_new_gui, where they fail:With the fix, all four of the reporting session's reproduction cases behave, and the two that must keep returning
Nonestill do:39 tests pass.
#alone is covered too — a bare#is a valid empty ATX heading,#xis not a heading at all.This PR body is the end-to-end proof. Its rationale below opens with an issue reference — the exact form that was rejected. The workflow checks out the PR's merge ref, so the gate that runs on this PR is the parser this PR ships. If the fix is wrong, this PR goes red on itself.
Model Used
Claude Opus 5 (1M context).
Single-issue rationale
#16104 alone. The other work in flight is a security-gate allowance (#16103), a CI trigger change (#16098) and a process doc (#16102) — different files, different risk, and batching a guard fix with a security allowance would put two unrelated review questions in one pass.
A correction this PR encodes. I had been telling four sessions today that a too-narrow exemption is self-correcting because it goes red. It goes red — that is not the same as going red informatively, and a red with a misleading cause is repaired by working around it rather than by fixing it. Reported by the session that hit the gate, whose reproduction is used verbatim as fixtures.