Skip to content

Add check-heading-levels - #1373

Merged
dem-extra1 merged 5 commits into
mainfrom
claude/project-thread-ykzrw4
Oct 30, 2026
Merged

dem-extra1 merged 5 commits into
mainfrom
claude/project-thread-ykzrw4

Conversation

@dem-extra1

Copy link
Copy Markdown
Collaborator

Requested by Ezra · project thread

Closes #1372.

Adds check-heading-levels, a diff-scoped, warn-only composite action and reusable workflow. On added heading lines it flags:

  • a heading more than one level deeper than the one before it in the same file;
  • a page whose first heading (following include shortcodes) is deeper than level 2;
  • a first heading that repeats the page title:.

Headings inside fenced divs (the #### titles of definition and exercise divs), code fences, comments and front matter do not count, and <!-- check-heading-levels: allow --> exempts a line. A page is a .qmd with a title: in its front matter, so include fragments take their level from the page that includes them.

Run in whole-tree mode on 2026-10-30 it finds nothing in lds and psw, and in the other notes repos only headings that the open fix PRs address or that remain open decisions:

  • rme: # Configuring R include, and five ### headings in count-regression.qmd that skip level 2.
  • mds: five pages that repeat their title as the first of several ## headings.
  • sds data.qmd and pds random-variables.qmd: title repeated as the first heading.

Tests: 24 unit tests (rules, exemptions, include resolution, diff scoping, skip-not-fallback, defaults); a mutation of the skip rule makes three of them fail. A heading-levels job in _selftest.yml exercises the composite action's inputs.

Also wired into the lists the repo requires for a new check: README.md, CLAUDE.md, website/ (permissions, versioning, workflows, reference page and sidebar), dependabot.yml, an examples/ stub and a changelog fragment. It exceeds the PR-size cap because a new check touches all of them.


Generated by Claude Code

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FWgYrDiMYAnjPMHWVTPJfZ
@dem-extra1 dem-extra1 self-assigned this Oct 30, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@dem-extra1

Copy link
Copy Markdown
Collaborator Author

🤖 Antigravity agent report

Cross-harness review (Gemini 3.8 Flash (Medium) via Antigravity agy 1.3.5, standing in while Argon quota is exhausted, read-only plan mode) at faabbfe

This supersedes the request at a65f26f.
I read and sent the model the three core files in full (the script, action.yml and the reusable workflow, 20 KB) with a list of things to check; I read the test file's test names myself (24 tests) and did not send the model the tests, the docs or the registration files, so this is not a read of those.
I checked each finding against the code; nothing was run.

Findings

  1. A page body can pass through an include that sits inside a fenced div.
    scan_headings tracks depth for fenced divs but expands {{< include >}} without checking it, and the included file's headings are parsed at depth = 0, so headings of a subfile that renders inside a callout or exercise div are counted, against the div exemption the docstring states.
  2. The moved-line rule silences breaks that depend on context, and it is untested.
    keep[rel] = {n for n, text in numbered.items() if text.strip() and text not in deleted} skips an added heading whose exact text was deleted elsewhere in the diff.
    A heading's validity depends on where it now sits (a ## Newton's method moved from a subfile into an aggregate file, for example, can now skip a level), and this check exists for those moves.
    action.yml documents the rule, but none of the 24 tests covers it; either drop the exemption for headings or test and justify it.
  3. scan_headings says one thing and scan_page does another.
    The docstring says the own flag lets "the skip rule ... compare headings of one file only", but scan_page updates prev on every heading, including those from includes, so an own heading is compared with the last heading an include rendered.
    That is a reasonable reading of the rendered outline; state it in the docstring.
  4. The include-cycle guard compares a real path with a relative one.
    resolve_include returns os.path.realpath(cand), but path (and so seen) holds the relative path the page was called with, so target not in seen and target != path do not recognise the top-level page; the recursion still ends once the real path is in seen, but a cycle back to the page is scanned once more and its headings are repeated.
    Compare real paths on both sides.
  5. A trailing # is dropped from a heading's text.
    In HEADING_RE, (\S.*?)[ \t]*#*[ \t]*$ strips closing hashes without requiring whitespace before them, so ## Learning C# is read as "Learning C"; this only matters for the title comparison.
    Require whitespace before the closing hashes.
  6. A multi-line YAML title is not read.
    TITLE_RE matches a one-line title:, so title: >- returns the text >- and the repeat-of-title test compares against that (a rare case).
  7. Every page re-reads and re-parses its includes.
    is_page reads every matched file, and shared includes are re-parsed for each page that includes them; memoise read_lines and the per-file parse (a performance note, not a bug at this size).

I disagree with the model's last point, which says the reusable workflow should call ./check-heading-levels instead of Morrison-Lab/gha/check-heading-levels@v3.
A local path in a reusable workflow resolves against the caller's checkout, not this repository, and the sibling check-one-div-per-subfile.yml at v3 uses the same Morrison-Lab/gha/<name>@v3 form.

Verdict

VERDICT: FINDINGS

Reviewed-Commit: faabbfe

Posted by Claude Code (AI agent) --- not written by a human.

Skip includes inside fenced divs, compare real paths for include cycles,
require whitespace before closing hashes, read folded multi-line titles,
cache file reads, and stop exempting moved heading lines.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FWgYrDiMYAnjPMHWVTPJfZ
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@dem-extra1

Copy link
Copy Markdown
Collaborator Author

🤖 Antigravity agent report

Cross-harness review (delta-focused, Gemini 3.8 Flash (Medium) via Antigravity agy 1.3.5, standing in while Argon quota is exhausted, read-only plan mode) at 771146e

This supersedes my read of faabbfe and covers the compare faabbfe...a190838 (the script and its tests, which I gave the model) plus a190838...771146e (one description line in action.yml, which I read myself).
I checked the model's findings against the code at this head; nothing was run.

Fixed since faabbfe: includes inside a fenced div are no longer expanded (depth == 0); the cycle guard compares real paths (seen holds real paths and the test covers a symlink cycle); a closing # now needs whitespace before it, so ## Learning C# keeps its #; a folded or literal YAML title is read; read_lines is cached on the file's path, modification time and size; the scan_headings docstring now says that scan_page compares across includes; the moved-line exemption is removed (a moved heading is flagged like a new one), and the stale line in action.yml now says so.

Findings

  1. The new moved-heading test does not prove the change.
    test_moved_heading_is_flagged_like_a_new_one rewrites ### C as #### C and moves ## B, so the added #### C has text that was not deleted, and the old code (with the exemption) flagged it at line 7 too; the test passes before and after the change.
    Use a heading whose text is unchanged: base ## A, ### X, #### C; new ## A, #### C, ### X, where the old code would not flag the moved #### C and the new code flags it at line 7 as a skip (check with git diff -U0 that git treats #### C, not ### X, as the moved line, since either choice is a valid diff).

I disagree with two of the model's points.
It says symlink cycle detection is still broken because target is not canonicalised, but resolve_include returns os.path.realpath(cand), so target and seen are both real paths, and test_include_through_a_symlink_cycle_stops exercises it.
It also says page_title conflates literal and folded block scalars, but joining the lines with a space is the folded form and a close enough reading of a literal one for comparing a title with a heading, so I leave it.

Verdict

VERDICT: FINDINGS

Reviewed-Commit: 771146e

Posted by Claude Code (AI agent) --- not written by a human.

@github-actions

Copy link
Copy Markdown
Contributor

Claude finished review — View run


I read PR #1373 live just now. It is OPEN, not merged (mergedAt is null), and its head is 466fbf0bbc3301f974b18dc9b59289036394cdaf, the commit I reviewed.

The prior verdict, Ready for merge on 771146e (comment), still stands. The only change since that round is a test-only edit. By hand-tracing, the rewritten moved-heading test yields exactly one deep finding at line 5. I could not run pytest in this job. I did not check whether git keeps ### X as an added line, and I did not mutation-test the case.

Verdict

Ready for merge

Structured Review Data (JSON)
{
  "schema_version": "1.1",
  "reviewer": "claude",
  "commit_sha": "466fbf0bbc3301f974b18dc9b59289036394cdaf",
  "verdict": "CLEAN",
  "findings": [],
  "detailed_assessment": "Traced the rewritten moved-heading test by hand: the moved first heading at line 5 yields exactly one deep finding, which would vanish if the moved-line exemption returned; pytest was unavailable so it was not run.",
  "holistic_assessment": "The only change since the last round is a test-only edit that makes the moved-heading case discriminate; production code, docs and workflow wiring are untouched, so the prior clean verdict stands."
}

Stopping Point: Clean stopping point reached --- session done; UMS executed; no follow-up items pending

Reviewed commit: 466fbf0

Reviewed commit: 466fbf0

@github-actions

Copy link
Copy Markdown
Contributor

💰 Cost: $0.7655 (review) — run

@dem-extra1

Copy link
Copy Markdown
Collaborator Author

🤖 Antigravity agent report

Cross-harness review (delta-focused, Gemini 3.8 Flash (Medium) via Antigravity agy 1.3.5, standing in while Argon quota is exhausted, read-only plan mode) at 466fbf0

This is a read of the compare 771146e...466fbf0 (one hunk, the moved-heading test), with my finding from the read of 771146e in the prompt: the old test passed with or without the moved-line exemption.
The new test puts ### X at the end of the base page and at the top of the new one, with a five-line prose block under ## A so that git treats ### X as the moved line.
I ran git diff -U0 on exactly those two files (on a Windows machine, with git's default diff): it reports ### X and a blank line as added at lines 5 and 6, and a blank line and ### X as deleted at lines 12 and 13, so the added heading's text is in the deleted set, which the old exemption skipped, and the new script flags it at line 5 as deep (the first heading of the page is level 3).
So the expected value [(5, "deep")] is right, the test fails against the old exemption and passes against the new script, and the comment explains why the prose block is there.
The model reached the same verdict and found nothing else.
The earlier fixes and the single description change at 771146e are unchanged.
I did not run the test suite.

Verdict

VERDICT: CLEAN

Reviewed-Commit: 466fbf0

Posted by Claude Code (AI agent) --- not written by a human.

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.

2026-10-30 21:44 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add check-heading-levels: skipped levels, deep first headings, repeated titles

2 participants