Widen Undeclared-Heading Advisory to copilot-instructions.md - #900
Merged
Conversation
The undeclared-H2 advisory (spec/section-model.md) only scanned AGENTS.md and GOVERNANCE.md, so a repo's own content sitting in .github/copilot-instructions.md was invisible to it, which is how ptr727/PhotoCleaner's local sections went undetected and duplicated a later OPERATIONS.md (#523). Extend UNDECLARED_HEADING_SCANNED to include copilot-instructions.md, which already carries a declared section list in files.json. Do not attempt to name a destination file for an undeclared heading: neither OPERATIONS.md's six headings nor ARCHITECTURE.md's are declared as data anywhere, and PhotoCleaner's actual headings ("Development Workflow", "Command Line Usage") matched neither, so a heading-name match would have missed the case that motivated this. The finding stays structural, naming the heading as undeclared and pointing at section-model.md's destinations for a human to judge, per #523's own warning against a content-similarity heuristic. Also make the H2 scan fence-aware via the existing unfenced_text helper, extracted the scan into undeclared_h2_headings() so it is unit-tested, and added 6 selftest cases covering the new scope, the fence fix, and the existing AGENTS.md/GOVERNANCE.md behavior.
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
PR Summary by QodoScan copilot-instructions.md for undeclared H2 headings (fence-aware)
AI Description
Diagram
High-Level Assessment
Files changed (2) |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The scope expansion and fence-aware fix are low-risk and come with selftest coverage; the remaining feedback is a minor helper-contract tightening.
Pull request overview
Extends the undeclared level-two heading advisory in the spec audit to also scan .github/copilot-instructions.md, and makes the heading scan fence-aware to avoid false positives from fenced code samples.
Changes:
- Widened the undeclared-heading advisory scope to include
.github/copilot-instructions.md. - Refactored the advisory into
undeclared_h2_headings()and made it ignore fenced code blocks viaunfenced_text(). - Added table-driven
--selftestcoverage for the advisory behavior (scope, casing, H2-only, fence handling).
File summaries
| File | Description |
|---|---|
| spec/section-model.md | Documents that the undeclared-heading advisory also applies to .github/copilot-instructions.md and clarifies why it remains structural-only. |
| spec/audit.py | Implements the widened scope, adds a fence-aware helper for undeclared H2 headings, and adds selftests covering the new behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Copilot review: the docstring said the function reads lowercased declared, but it trusted the caller to have already normalized it. Normalize inside the helper so the contract holds for any caller, plus a selftest case with an un-normalized declared set. Also parenthesize the implicitly-concatenated string literal in the new selftest fixture (ruff ISC004), caught by CI, not by the local ruff-less environment this was authored in.
Code Review by Qodo
1. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
CI's format-check job caught this, not the check job: the multi-line set comprehension fits ruff's line-length on one line.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, well-scoped, and includes selftests; the only feedback is a minor docstring/implementation mismatch note.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
spec/audit.py:463
- The docstring says the normalization is “case-folded”, but the implementation uses .lower(), which is not full Unicode case-folding (and makes the docstring inaccurate). Use .casefold() for both extracted headings and declared names (or update the docstring to say “lowercased”).
h2s = {ln[3:].strip().lower() for ln in unfenced_text(text).split("\n") if ln.startswith("## ")}
return sorted(h2s - {d.strip().lower() for d in declared})
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Copilot suppressed finding: the docstring said case-folded but the code uses .lower(), and every other case-insensitive comparison in this file (heading_texts, hub-name matching) already says lowercased in its own docstring. Match that vocabulary rather than switch this one call site to .casefold() and diverge from the rest of the file.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, reuses the existing unfenced_text() behavior to avoid false positives, and adds selftests covering the new scope and the fenced-heading parsing fix.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
ptr727
commented
Aug 21, 2026
OwnerAuthor
Answering the suppressed finding from the round on spec/audit.py:463 — "The docstring says the normalization is 'case-folded', but the implementation uses Fixed in 3eed107: corrected the docstring to say "lowercased" rather than switching the implementation to |
qodo-code-review (advisory, under evaluation per docs/pr-reviewer-evaluation.md) flagged real violations: - Comments referencing #523: task context belongs in the PR description and commit history, not in long-lived code comments. - The undeclared-section advisory's existing 5-line comment block grew by 2 lines instead of staying the same length; restored it to 5 lines with the scope note folded into the existing line. - A new 3-line comment where the sibling TEMPLATE_REF_SCANNED constant sets a 1-line precedent; matched it. - A spaced hyphen and a semicolon in the new docstring, both banned in agent-authored prose regardless of the syntax carrying them.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The scope expansion is narrowly implemented, the fence-awareness fix reduces false positives, and the added selftests cover the new behavior.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 22, 2026
Merged
ptr727 added a commit
that referenced
this pull request
Aug 22, 2026
## Summary Follow-up to #900, addressing findings raised on the #900 -> #901 promotion PR review (qodo-code-review and CodeRabbit, both advisory reviewers on this PR since CodeRabbit is only enabled against `main` as a base). - **Real bug, verified independently**: `unfenced_text()` toggled its fenced state on *any* line starting with `` ``` `` or `~~~`, regardless of marker family or length. A `~~~` line nested inside a `` ``` `` block closed the wrong fence, and a shorter `` ``` `` inside a longer `` ```` `` closed a fence it should not have been able to close. Confirmed both failure modes against the actual code before fixing, and fixed per CommonMark: a fence closes only on the same marker character, at least as long as the opener. `unfenced_text` is a pre-existing helper several other checks (README shields/links) already depend on, so this fixes it for all of them, not just the new undeclared-heading advisory. Added 5 regression cases; the full existing selftest suite (readme shields/links) still passes unchanged. - **Real duplication**: `TEMPLATE_REF_SCANNED` and `UNDECLARED_HEADING_SCANNED` were two identical tuples that could silently drift apart on a future edit to one and not the other. Made the second an alias of the first. - **Real prose issues**: two over-length sentences in `section-model.md`, one of them also past-tense change-framing ("has accumulated undetected before" -> present tense), plus three over-25-word sentences in new `audit.py` comments/docstrings. Split per `comment-and-doc-style`. Two findings from the same review round were judged not real and declined in the PR conversation on #901 with evidence, no code change: a PR-title Title-Case false positive (the cited rule actually allows lowercase "to"), and a "docstring too internal" finding contradicted by existing precedent in the same file (`strip_sections`' docstring already names `extract_section`). ## Verification - `python3 spec/audit.py --selftest` -> `SELFTEST PASS`, including the 5 new `unfenced_text` cases and the full pre-existing suite unchanged - `python3 spec/validate.py` -> `Spec validation OK` - `ruff check .` / `ruff format --check .` -> clean - `python3 scripts/prose_lint.py --diff origin/develop spec/audit.py spec/section-model.md` -> clean <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified that undeclared-heading checks include `.github/copilot-instructions.md`. * Documented heading-scan behavior, including H2-only matching, normalization, and fenced-content handling. * **Bug Fixes** * Improved fenced-content detection to correctly recognize compatible closing fences. * Ensured undeclared-heading scans consistently use the configured template references. * **Tests** * Added coverage for fence rules and related heading-scan behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ptr727 added a commit
that referenced
this pull request
Aug 22, 2026
## Summary Follow-up to #902, addressing a finding raised on the #900 -> #901 promotion PR review (CodeRabbit). `unfenced_text()` (fixed in #902) now handles fence marker family, length, and indentation correctly, but `extract_section()` and `strip_sections()` still used the original naive toggle-on-any-marker logic. Verified independently before fixing: a `~~~` line nested inside a `` ``` `` block made both exit the fenced state early, so a following `## ` line could end the region short. This is not cosmetic for `extract_section()`: it is what the verbatim byte-for-byte section check hashes, so a nested example inside a fenced code sample could silently truncate what gets compared against the hub canonical. Extracted the corrected per-line fence logic from `unfenced_text()` into `_fence_step()`, a single pure function all three now call, so the fence-matching rule lives in exactly one place instead of three near-duplicates that can drift apart the way the first two already had. Added a regression case covering the nested-marker scenario for both functions, on top of the existing `extract_section` and `strip_sections` (via `template_ref_outside_verbatim`) coverage, which still passes unchanged. ## Verification - `python3 spec/audit.py --selftest` -> `SELFTEST PASS`, including the new nested-fence regression and the full pre-existing suite unchanged - `python3 spec/validate.py` -> `Spec validation OK` - `ruff check .` / `ruff format --check .` -> clean - `python3 scripts/prose_lint.py --diff origin/develop spec/audit.py` -> clean
ptr727 added a commit
that referenced
this pull request
Aug 22, 2026
## Summary Follow-up to #903, addressing a finding raised on the #900 -> #901 promotion PR review (CodeRabbit). Per CommonMark, a backtick-fenced opener's info string may not itself contain a backtick (the spec's own reasoning: otherwise inline code spans could be misread as a new fence). \`_fence_step()\` accepted an opener like `` ```md` `` regardless, so a heading right after it was hidden from the scan. Verified independently against the actual code before fixing. A tilde fence has no such restriction and is unaffected. Also split the two over-25-word docstring sentences flagged in the same review round. ## Verification - `python3 spec/audit.py --selftest` -> `SELFTEST PASS`, including 3 new regression cases and the full pre-existing suite unchanged - `python3 spec/validate.py` -> `Spec validation OK` - `ruff check .` / `ruff format --check .` -> clean - `python3 scripts/prose_lint.py --diff origin/develop spec/audit.py` -> clean ## A note on scope This is the fourth follow-up PR (#901 -> #902 -> #903 -> this one) chasing progressively deeper CommonMark fence-parsing edge cases that CodeRabbit's automated review keeps finding one round at a time against `_fence_step()`. Each one has been real and independently verified, but I want to flag the pattern rather than silently keep going: CommonMark has more edge cases than these four (unterminated fences at EOF, tab expansion in indentation, and others), and a sufficiently persistent automated reviewer may keep surfacing them. Worth a decision on where "correct enough" is for a fleet-internal audit tool versus a full CommonMark implementation.
ptr727 added a commit
that referenced
this pull request
Aug 22, 2026
## Summary Follow-up to #904, addressing a suppressed Copilot finding raised on the #900 -> #901 promotion PR review. `undeclared_h2_headings()`'s docstring read as if a bare \`##\`-prefixed shell comment is never misread as a heading anywhere, when that only holds inside a fenced code sample, the same as the heading-syntax example right beside it. Reworded so both read as one example of fenced content rather than two independent claims. ## Verification - \`python3 spec/audit.py --selftest\` -> \`SELFTEST PASS\` - \`python3 spec/validate.py\` -> \`Spec validation OK\` - \`ruff check .\` / \`ruff format --check .\` -> clean - \`python3 scripts/prose_lint.py --diff origin/develop spec/audit.py\` -> clean
ptr727 added a commit
that referenced
this pull request
Aug 22, 2026
## Summary - Widen the undeclared-H2 advisory to scan `.github/copilot-instructions.md`, so a repo's own content sitting there is no longer invisible to the audit. ## Included Work - `03e88e8` Widen Undeclared-Heading Advisory to copilot-instructions.md (#900). ## Tracking Fixes#523. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved documentation audits to detect undeclared H2 headings across all supported instruction and governance files. - Heading checks now ignore fenced code, normalize spacing and capitalization, and exclude unrelated heading levels. - Improved handling of fenced content, including marker type, length, indentation, and trailing text. - **Documentation** - Updated enforcement guidance to include repository instruction files and clarify where undeclared headings should be documented. - **Tests** - Added coverage for nested, fenced, case-insensitive, whitespace-normalized, declared, and undeclared headings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Extends the undeclared-H2 advisory (
spec/section-model.md) to scan.github/copilot-instructions.md, not justAGENTS.mdandGOVERNANCE.md. That gap is whyptr727/PhotoCleaner's repo-specific sections went undetected and duplicated a laterOPERATIONS.md.What this does and does not do
Re-measured against current
developbefore starting: nothing has touchedspec/audit.py,spec/section-model.md, orspec/files.jsonsince the most recent comment on #523, so the gap it describes is current, not stale.UNDECLARED_HEADING_SCANNEDnow includes.github/copilot-instructions.md, which already carries a declared section list infiles.json.OPERATIONS.md's six headings norARCHITECTURE.md's are declared as data anywhere today, and PhotoCleaner's actual headings (Development Workflow,Command Line Usage) matched neither name, so a heading-name match would have missed the exact case that motivated the issue. The finding stays structural — it names the heading as undeclared and points atsection-model.md's destinations for a human to judge — consistent with the issue's own warning against a content-similarity heuristic. Declaring a heading-to-destination vocabulary (issue open question 2) and reachingARCHITECTURE.md(open question 3) stay open.unfenced_texthelper, so a##line inside a fenced code sample could misread as a real heading. Extracted intoundeclared_h2_headings()and fixed while touching this code, since expanding scope to a file more likely to carry fenced examples made the gap more likely to bite.--selftestcases (scope, case-insensitivity, H2-only, the fence fix, and the new copilot-instructions.md case).Verification
python3 spec/audit.py --selftest→SELFTEST PASSpython3 spec/validate.py→Spec validation OKpython3 scripts/prose_lint.py --diff origin/develop spec/audit.py spec/section-model.md→ cleanCloses#523.