Skip to content

docs: default tables of contents to one level - #1324

Open
aparajon wants to merge 5 commits into
mainfrom
armand/toc-depth
Open

docs: default tables of contents to one level#1324
aparajon wants to merge 5 commits into
mainfrom
armand/toc-depth

Conversation

@aparajon

@aparajon aparajon commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Generated tables of contents list every subsection, taking up too much space before readers reach the guide. Default to main sections (## headings) and regenerate existing TOCs with the shorter layout.

Documents can opt into more detail with max-depth=3 in the opening TOC marker. The setting survives regeneration and CI checks, supports heading levels 2–6, and preserves duplicate-heading anchors. Only TOC entries change; the document sections stay intact.

Validation: six regression tests covering depth settings, regeneration, invalid values, and duplicate anchors; Go build and documentation checks passed.

Generated with Codex

Copilot AI lite review requested due to automatic review settings September 7, 2026 03:50
@aparajon
aparajon marked this pull request as ready for review September 7, 2026 03:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The TOC generator currently drops an explicit max-depth=2 marker during regeneration, making --check non-idempotent for that valid configuration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates the documentation TOC generator to default to listing only top-level (H2) sections, while allowing individual documents to opt into deeper TOCs via a max-depth marker attribute, and refreshes existing docs to match the new default.

Changes:

  • Default TOC generation to include only H2 headings unless max-depth is specified (2–6), and preserve that setting through regeneration/checks.
  • Add Python regression tests covering default behavior, opt-in depth, idempotency, duplicate-anchor numbering, and invalid values.
  • Regenerate TOCs across docs to remove subsection entries under the new default.
File summaries
File Description
scripts/test_gen_doc_toc.py Adds regression tests for per-document TOC depth behavior.
scripts/gen-doc-toc.py Implements max-depth parsing and default depth behavior in TOC generation/checking.
docs/strata-engine.md Refreshes TOC to top-level sections only.
docs/spirit_progress.md Refreshes TOC to top-level sections only.
docs/schema-intelligence.md Refreshes TOC to top-level sections only.
docs/postgresql.md Refreshes TOC to top-level sections only.
docs/pending-drops.md Refreshes TOC to top-level sections only.
docs/namespaces.md Refreshes TOC to top-level sections only.
docs/invariants.md Refreshes TOC to top-level sections only.
docs/grpc-control-edge-cases.md Refreshes TOC to top-level sections only.
docs/github-app-setup.md Refreshes TOC to top-level sections only.
docs/engines.md Refreshes TOC to top-level sections only.
docs/configuration.md Refreshes TOC to top-level sections only.
docs/check-runs.md Refreshes TOC to top-level sections only.
docs/architecture.md Refreshes TOC to top-level sections only.
docs/apply-lifecycle.md Refreshes TOC to top-level sections only.
Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/gen-doc-toc.py Outdated
@Kiran01bm

Copy link
Copy Markdown
Collaborator

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1324, 1840cbb.

Verdict: 8 findings — 0 blocking, 6 non-blocking, 2 suggestions. Re-reviewed the delta e2da919c → 1840cbbe: the tool files are byte-identical across it, and the one new commit (9 H3 TOC lines removed from docs/pre-merge-workflow.md) was a required rebase fix, not churn — I reproduced check-docs-toc failing at 1840cbbe~1 with stale TOC: docs/pre-merge-workflow.md and passing at head, and the 9 lines are byte-exactly generator output. Nothing new turned up in the delta; the findings below are the earlier review's, re-verified at this head. The depth mechanism itself works: depths 3, 4 and 6 all round-trip idempotently against real docs and duplicate-anchor numbering still matches GitHub.

Non-blocking

1. An explicit max-depth=2 is silently deleted, and --check then reports the doc stale with a byte-identical TOC. :131 reads begin = BEGIN if max_depth == 2 else BEGIN.replace(...), so depth 2 can never round-trip its own marker. Reproduced end to end: marker rewritten, TOC body untouched, --checkstale TOC exit 1 — and the printed remedy "fixes" it by discarding the author's directive. The docstring at :16-17 ("Depth is an absolute heading level from 2 to 6. The setting survives both regeneration and check mode") is therefore false for a value it names as legal.

2. A case typo bypasses the typo-rejection path the PR just added. :126 guards on "max-depth" in marker.group() — a case-sensitive substring test — so MAX-DEPTH=3 falls through to depth 2 with no error, and the annotation is then erased from the marker. Verified: lowercase max-depth=9 correctly raises TOC max-depth must be a heading level from 2 to 6, while MAX-DEPTH=3 and Max-Depth=3 both produce an H2-only TOC in silence. Lowercasing the haystack makes it either work or raise.

3. The feature ships with no automated coverage. The six new tests in scripts/test_gen_doc_toc.py are run by no make target, no workflow and no git hook — a repo-wide grep for unittest/pytest/discover hits only that file's own docstring, and Makefile:167-171 are the only python3 invocations anywhere. Meanwhile no committed doc uses max-depth, so the gate that does run (docs.yaml:28) never enters the new marker-parsing branch either. The suite would not have caught finding 1 regardless: test_depth_survives_regeneration asserts idempotence on regenerate's own output, which is already the normalized marker.

4. docs/invariants.md is the obvious first candidate for the new opt-in, and no document opts in. Its 72 H3s are the ID'd invariant entries (### AV-1: …, ### MG-5: …) and the 72 removed TOC lines map to them one-for-one — the largest loss of the 19 docs, next being check-runs.md at 25. AGENTS.md:153 mandates "Cite the invariant by ID in the PR summary and in review findings", so ID lookup over a 1348-line file is the repo's own prescribed workflow; its TOC now lists 16 family names. Softening it fairly: every H2 carries its family code, AGENTS.md:149 publishes the family map and the H3 anchors are untouched, so an ID stays a deterministic two-hop lookup — but one marker on invariants.md:3 would make the change right rather than merely defensible.

5. An invalid depth aborts the write sweep mid-flight, before the manifest is written. :250 returns 1 on the first ValueError, after earlier docs have already been rewritten and before write_manifest() runs. Reproduced with a two-doc fixture: one doc updated, no manifest written, and the next --check fails with an unrelated docs/.toc-manifest is missing — sending the author after the wrong problem. Validating all markers before any write makes the sweep all-or-nothing.

6. The broken-TOC message got less useful. :264 now reads "has no headings within its TOC depth", dropping both the fact that markers are present and — unlike every other operator message in this file (:199, :205, :211 all end in "run make docs-toc…") — any statement of what to do. It never names the depth, which is silently 2 in the common case. This matters more after this PR, because an H3-only doc that previously generated a fine TOC is now a hard CI failure.

General suggestions

7. build_toc's max_depth: int = 2 default at :104 is unreachable and untested. :143 is the only call site and always passes the value, so the default duplicates the real one at :125 in a second place; mutating it 2 → 3 survives all six new tests. Dropping the default parameter makes regenerate the single source of truth.

8. Docstring and ergonomics. The rewrap left a 137-char line at :10 in a docstring otherwise wrapped at ~75 (plus two more new lines over 88, where the merge base had one in the whole file); :9 shortened "skipping headings inside fenced code blocks" to "skipping fenced code blocks", which describes something the tool does not do. There is also no way to set depth at insert time, so the workflow for a deep TOC is insert, hand-edit tool-owned marker text, re-run — a --max-depth flag would fold that into one step and give both error messages somewhere to point.

The one thing that could have broken, verified

Both marker regexes use [^>]*, which cannot match > — so the hazard was that the strip regex at :134 would fail to match a longer ; max-depth=3) --> marker while the detect regex matched it, leaving the old block in place and appending a second one. That would be invisible to CI today, since no committed doc carries a max-depth marker. Tested rather than reasoned: on a real doc hand-edited to max-depth=3, the output had exactly one BEGIN, one END and one ## Table of Contents, marker preserved verbatim, second run byte-identical — [^>]* is greedy but backtracks over the two hyphens, so both regexes match. The only way to break it is a marker containing a literal >, which the tool never emits.

Verified correct

  • Delta e2da919c → 1840cbbe: scripts/gen-doc-toc.py and scripts/test_gen_doc_toc.py byte-identical; the only new commit is 9 deletions inside one TOC marker block.
  • The delta was mandatory: check-docs-toc fails at 1840cbbe~1 (stale TOC: docs/pre-merge-workflow.md) and passes at head — the rebase pulled in a doc carrying an old-tool H2+H3 TOC.
  • The 9 removed lines are byte-exactly generator output: 0 H2 entries lost, 0 anchors altered, all 9 H3 headings still in the body, and the doc's own #if-the-check-is-stuck link at :217 still resolves.
  • Depths 3, 4 and 6 round-trip idempotently and survive --check; depth 7 and the malformed forms (62, 3x, 03, 1, 20, two, empty) all raise with the intended message.
  • Duplicate-anchor numbering still matches GitHub, including the -1 slot reserved for a depth-excluded H3 and for headings inside fenced blocks.
  • 5 of 6 source mutants killed by the new tests, including dropping the marker rewrite, widening [2-6] to [1-9], and removing the lookahead.
  • make check-docs-toc clean over all 19 files and make docs-toc a no-op — the tree is at a fixed point, with no broken-TOC reports; .toc-manifest's 19 entries match the 19 marker docs.
  • The sweep is complete: no doc retains an indented H3 TOC entry, and every doc left alone has zero H3s so depth 2 was already its output.
  • Nothing depends on TOC shape — no #table-of-contents links, no deep links into removed entries, no tooling parses the block; README.md:38's only inbound anchor targets a retained H2.
  • CI green at this head, including Docs TOC, Terminology and Templates drift.

This review was generated by Claude Code (claude-opus-5).

@aparajon

aparajon commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Addressed in b407e49:

  • Preserve explicit max-depth=2 and reject case typos instead of silently dropping them
  • Run the regression suite through make check-docs-toc, which CI already invokes
  • Validate all selected markers before the write sweep, so an invalid setting cannot leave partially updated docs
  • Include the effective depth and recovery guidance in the broken-TOC error, and remove the redundant build_toc default

Nine tests pass, including an end-to-end --check with explicit depth 2 and a sweep that verifies invalid settings leave files untouched. Merge conflicts are also resolved.

Keeping invariants.md at one level is intentional: the goal is a compact overview across the docs, with the existing section anchors still available. Deeper TOCs remain an opt-in. A CLI depth flag would be a useful follow-up, but this PR keeps configuration in each document's marker.

@Kiran01bm

Copy link
Copy Markdown
Collaborator

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1324, b407e49.

Verdict: 7 findings — 0 blocking, 5 non-blocking, 2 suggestions. Re-reviewed the delta 1840cbbe → b407e491 (a rebase onto main plus one rework commit). Six of the seven issues from the previous round are genuinely fixed, and I verified each rather than taking the commit message's word for it: depths 2–6 now all round-trip byte-identically and idempotently, a case typo raises instead of silently defaulting, the write sweep is all-or-nothing in both file orderings, and the tests are now a required CI check. What is left is narrower — an error message that gives wrong advice, and one reporting path the new tests do not reach.

Non-blocking

1. One ValueError covers eight distinct faults, and its advice is wrong for most of them. :129 raises "use lowercase max-depth=N with a heading level from 2 to 6" for wrong case, out-of-range (=7, =1), a malformed or absent value (=3x, =, bare max-depth, max-depth = 3 with spaces), and a duplicate key. Measured: all eight inputs produce the byte-identical message, so a user who wrote correct lowercase max-depth=7 is told to "use lowercase". Only 3 of 14 rejection inputs get advice that describes their actual fault.

2. The broken-TOC report is the one path the new tests never reach, and all three of its defects survive. Hardcoding H2–H3 in place of the interpolated depth at :281 passes all 9 tests. The message renders has no H2–H2 headings to list at the default depth, and it advises set max-depth=3 (up to 6) even for a doc whose marker already says max-depth=4 — where following it would narrow the range. A CLI test asserting no H2–H4 headings on a max-depth=4 doc with only an H5 closes all three.

3. The duplicate-key guard is untested. Dropping or marker.group().lower().count("max-depth") != 1 from :128 passes all 9 tests, after which max-depth=3; MAX-DEPTH=4 is silently accepted as depth 3 and the contradictory annotation is rewritten away. One assertRaises on that exact marker kills it.

4. The test file documents an invocation that permanently dirties the repo. test_gen_doc_toc.py:1 says to run python3 -m unittest discover … with no -B, while Makefile:171 correctly uses -B — and .gitignore at this head has no __pycache__/*.pyc entry. Reproduced: running the docstring's command verbatim creates scripts/__pycache__/test_gen_doc_toc.cpython-314.pyc, which git check-ignore does not match, so it sits in git status forever. Add -B to the docstring and __pycache__/ to .gitignore — the latter also covers the two subprocess children, which do not inherit -B (harmless only because they run as __main__ in a temp cwd).

5. Still no document opts in, and docs/invariants.md is now the clearest case it has ever been. Re-counted at this head: 1361 lines, 17 H2s, and exactly 73 H3 TOC entries removed — 5.5× the next-largest loss (check-runs.md at 25), and a pure deletion with zero insertions. The new argument is the anchor shape: a slug embeds the full title (#mg-5-apply-owned-check-rows-are-released-only-by-their-owner), so you cannot construct the deep link from an ID alone — which makes the two-hop lookup mandatory for exactly the workflow AGENTS.md:153 prescribes ("Cite the invariant by ID in the PR summary and in review findings"). Fairly stated: every family H2 carries its code and the H3 anchors are untouched, so nothing breaks — but one line on invariants.md:3 would make the change right rather than merely defensible, and would give the feature its first real user.

General suggestions

6. The try/except ValueError in the processing loop is now dead. :261-265 can only fire for a marker the validation pass at :246-254 did not already reject, and the two predicates are provably the same expression (explicit or has_toc_markers versus its negation inside process). Instrumented and probed with nine escape attempts — sweep, explicit, duplicate paths, aliases, a symlink, a path outside docs/ — and the handler never fired; only an external concurrent write could reach it. Fine as defence in depth, but worth a comment saying so, and note that non-ValueError faults (missing path, directory) escape both handlers as OSError.

7. Style debt that survived two reworks. :10 is still a 137-character line in a docstring otherwise wrapped at ~75, and :9 still says "skipping fenced code blocks" where the tool actually skips headings inside them. There is also still no --max-depth flag, so setting a depth means inserting the block, hand-editing tool-owned marker text, and re-running — the one step the new toc_settings plumbing would make trivial to expose.

The one thing that could have broken, verified

Marker discovery is not fence-aware: both toc_settings and the strip regex at :139 re.search the whole file, so a doc containing a fenced example marker can have the wrong one matched — and now that markers carry a depth payload, the wrong marker is semantically wrong, not merely cosmetically wrong. I built both hazards. With a fenced max-depth=4 example above the real block, depth is read as 4, the non-greedy strip swallows the closing fence, the unclosed fence hides every heading, and the file reports broken TOC … has no H2–H4 headings forever while plainly containing ## Alpha — unfixable by make docs-toc. With a fenced BEGIN…END pair, the example's body is silently deleted and the result passes --check. Then I ran the same two fixtures against the merge base c8a75527 and got the same wedge, the same exit codes, and — in the natural layout, marker at line 3 as in all 19 committed docs — a byte-identical destroyed file. So this is pre-existing, not introduced here; the PR's only amplification is that a stray marker's setting now gets promoted into the real marker. No committed doc is exposed: all 19 have exactly one marker, all at line 3, and no doc under docs/ documents the marker syntax at all.

Verified correct

  • Previously-broken depth 2 now round-trips: the marker is preserved byte-identically, a second run is idempotent, and --check exits 0. Same for depths 3, 4, 5 and 6.
  • MAX-DEPTH=3, Max-Depth=3 and max-DEPTH=3 now raise instead of silently falling back to depth 2 — the previous silent-typo path is closed.
  • The validation pass makes the write sweep genuinely all-or-nothing: with the invalid file sorting first and last, every valid doc stays byte-unchanged, no manifest is written, and a pre-existing manifest is untouched. Confirmed in --check mode too.
  • The tests are now wired into CI and blocking: Makefile:171 runs them as the first recipe line of check-docs-toc, both recipe lines are unprefixed by -, and "Docs TOC" is a required status check on main.
  • The suite is stdlib-only (runpy, subprocess, tempfile, sys, unittest, pathlib) and needs no setup-python step; the repo has no other Python test, so this PR establishes the pattern.
  • build_toc's unreachable max_depth default is gone, so toc_settings is the single source of truth for the value.
  • 7 of 9 targeted mutants die, including dropping .lower(), reverting the depth-2 preservation, deleting the validation loop, widening [2-6] to [1-9], removing the lookahead, and turning the raise into a silent default.
  • make check-docs-toc is clean over all 19 files and make docs-toc is a no-op; -B leaves no __pycache__ behind in the CI path.
  • No H3 TOC entry remains in any committed TOC — the sweep is complete, and the four docs left alone have no H3s.
  • The PR owns only Makefile, 15 docs and the two scripts/ files at this head; none of main's rebased files were touched.
  • CI green at this head, including Docs TOC, Terminology, Templates drift and Unit Tests.

This review was generated by Claude Code (claude-opus-5).

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.

3 participants