Add heading-max, code-block-max, and stern mode to line-length rule - #1
Conversation
Extend TM001 line-length rule with per-category maximum lengths and stern mode, matching markdownlint MD013 feature set: - heading-max: override max for heading lines (uses AST detection) - code-block-max: override max for code block lines - stern: only flag lines with a space past the limit column Includes unit tests (26 new), integration fixtures, and updated rule documentation. https://claude.ai/code/session_01PNAbRKSSUqz9rtkM37eKiM
|
@copilot fix ci |
There was a problem hiding this comment.
Pull request overview
This PR enhances the line-length rule (TM001) with three new configuration options to provide finer-grained control over line length checking: per-category limits for headings and code blocks, and a "stern mode" that only flags lines with spaces past the limit.
Changes:
- Added
heading-max,code-block-max, andsternsettings to the Rule struct with corresponding validation and application logic - Refactored line checking to pre-compute line categories and determine active limits based on line type
- Added comprehensive test coverage with 30+ unit tests and example markdown files demonstrating the new features
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/rules/linelength/rule.go | Implements per-category limits and stern mode logic; refactors settings application into focused helper methods; adds AST-based heading detection and category-based limit selection |
| internal/rules/linelength/rule_test.go | Adds comprehensive test coverage for heading-max, code-block-max, and stern mode features with tests for inheritance, composition, and edge cases |
| rules/TM001-line-length/README.md | Documents the new settings with descriptions, configuration examples, and usage examples for each feature |
| rules/TM001-line-length/good/stern-no-spaces-past-limit.md | Example demonstrating stern mode allowing long URLs without spaces past limit |
| rules/TM001-line-length/good/heading-within-limit.md | Example showing heading within heading-max limit |
| rules/TM001-line-length/good/code-block-within-limit.md | Example showing code block within code-block-max limit |
| rules/TM001-line-length/bad/stern-spaces-past-limit.md | Example showing stern mode flagging lines with spaces past limit |
| rules/TM001-line-length/bad/heading-over-limit.md | Example showing heading exceeding heading-max |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // "# " (2) + 58 'h' chars + space at position 60 + 5 'x' chars = 66 total. | ||
| // heading-max=60, so the space at position 60 is past the limit. |
There was a problem hiding this comment.
The comment describing the test case is misleading. It states "space at position 60" when using a heading-max of 60, but the space is actually at 0-indexed position 60, which is the 61st character (1-indexed). Since the limit is 60, this space is indeed past the limit. Consider clarifying: "# " (2 chars) + 58 'h' chars = 60 chars total, then space at 0-indexed byte 60 (past the limit), then 5 'x' chars, totaling 66 chars."
| // "# " (2) + 58 'h' chars + space at position 60 + 5 'x' chars = 66 total. | |
| // heading-max=60, so the space at position 60 is past the limit. | |
| // "# " (2 chars) + 58 'h' chars = 60 chars total, then a space at 0-indexed position 60 | |
| // (the 61st character, i.e., past heading-max=60), then 5 'x' chars, for 66 chars total. |
Good test fixtures use non-default settings via front matter that are only applied by the integration test framework, not by tidymark check. Exclude them from the CLI check to avoid false positives (affects TM001 stern, TM002 setext, TM010 tilde, TM016 spaces-4 fixtures). https://claude.ai/code/session_01PNAbRKSSUqz9rtkM37eKiM
- TM001: fix heading-max for Setext headings (include underline), update docs to say "ATX and Setext", fix example heading length, clarify stern+heading-max test comment - TM023/TM024: skip markdown tables (parsed as paragraphs by goldmark without table extension), add tests, update docs to note tables and code blocks are skipped - Add plan 46: design table readability measure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ran 5 simulated developer trials with a 15-snippet
questionnaire. Key findings: {{.field}} dual meaning is
the #1 confusion source, indented directives silently
break (avg confidence 2.6), nested directives are
undefined (avg 2.0), and ratio parameter name misleads.
Updated tasks to address all discovered misconceptions.
https://claude.ai/code/session_015XXFMqS3iqmbyMsNJFeCeY
Ran 5 simulated developer trials with a 15-snippet
questionnaire. Key findings: {{.field}} dual meaning is
the #1 confusion source, indented directives silently
break (avg confidence 2.6), nested directives are
undefined (avg 2.0), and ratio parameter name misleads.
Updated tasks to address all discovered misconceptions.
https://claude.ai/code/session_015XXFMqS3iqmbyMsNJFeCeY
- Remove includedDir != "." guard so root-level includes get source-dir injection (review comment #1) - Compute display prefix relative to the catalog-owning file's directory via filepath.Rel, so links are correct when the includer is in a subdirectory (review comments #2, #3) - Handle source-dir: "." by using RootFS directly - Remove unused resolveGlobMatches wrapper (lint fix) - Add tests for root-include and sibling-subdir edge cases - Update copilot-instructions.md (mdsmith fix) https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
- Remove includedDir != "." guard so root-level includes get source-dir injection (review comment #1) - Compute display prefix relative to the catalog-owning file's directory via filepath.Rel, so links are correct when the includer is in a subdirectory (review comments #2, #3) - Handle source-dir: "." by using RootFS directly - Remove unused resolveGlobMatches wrapper (lint fix) - Add tests for root-include and sibling-subdir edge cases - Update copilot-instructions.md (mdsmith fix) https://claude.ai/code/session_01VN3XGWEbs4qRPet8qBia59
Address PR #215 review (threads on test lines 258 and 309): the fixPassProbeRule was returning a diagnostic on Check call #1, which is the pre-fix engine.CheckRules pass — applyFixPasses then saw an empty diagnostic list on call #2 and never invoked Fix. So the \"validates hydration during applyFixPasses\" assertion was actually just validating hydration during pre/post-fix CheckRules, which the other tests in this file already cover. Rework: - Track Check and Fix snapshots separately. - Trigger the diagnostic on Check call #2 (the applyFixPasses pass) so Fix actually fires, then assert exactly 1 Fix snapshot. - Pin exact phase counts: 3 Check calls, 1 Fix call. - Assert the Fix call's lint.File also has the per-file context hydrated, not just the Check calls — that's the real regression guard for catalog/include rules whose Fix paths consult these fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Suppress diagnostics inside generated sections during fix The Fixer's pre-fix and post-fix CheckRules calls were running on *lint.File values whose GeneratedRanges had never been populated, so filterGeneratedDiags was a no-op. As a result, `mdsmith fix` surfaced diagnostics inside <?catalog?> / <?include?> bodies that `mdsmith check` correctly hid — the same source bytes produced different results depending on which command ran them, which broke the merge queue's pre-merge-commit hook. Why: the runner sets GeneratedRanges before linting (runner.go:108, :201); the Fixer didn't, leaving range-based filtering inert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Propagate parse-time fields onto post-fix lint.File Address PR #215 review: the post-fix CheckRules call also needs StripFrontMatter and MaxInputBytes from the pre-fix lf, not just FrontMatter/LineOffset. Without them, rules that read secondary files (catalog, include, requiredstructure, crossfilereferenceintegrity) or align cross-file coordinates (duplicatedcontent) silently behave differently between the pre-fix and post-fix passes — the same kind of runner/fixer divergence the GeneratedRanges propagation addressed. Extract the post-fix file construction into buildPostFixFile so all parse-time and resolution context lives in one place, and so the next field added to lint.File doesn't get forgotten by one of the call sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Wire GitignoreFunc into Fixer for runner parity Address PR #215 review: the catalog rule's resolveGitignore calls f.GetGitignore() to filter glob hits, but the Fixer never set GitignoreFunc on its lint.File. So a catalog directive whose glob matched gitignored files would silently include them when fix regenerated the body, but exclude them when check ran on the same bytes — the same flavor of fix/check divergence the GeneratedRanges and parse-time-field propagations addressed. Mirror engine.Runner: per-dir cached GitignoreMatcher, closure captured on lf in prepareFile, propagated to finalFile in buildPostFixFile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Hydrate per-file context inside applyFixPasses Address PR #215 review (thread on internal/fix/fix.go:200): the parsedFile that fixable rules see inside applyFixPasses was only getting FS/RootDir wired up, so catalog.Fix (calls f.GetGitignore) and include.Fix (uses f.MaxInputBytes) silently produced different post-fix bytes than `mdsmith check` would have validated against. Extract a hydrateLintFile helper that copies onto a freshly-parsed *lint.File the full per-file context the engine.Runner sets: FS / RootFS / RootDir / FrontMatter / LineOffset / StripFrontMatter / MaxInputBytes / GitignoreFunc / GeneratedRanges. Use it in both applyFixPasses (for parsedFile) and buildPostFixFile (for finalFile) so all three lint passes — pre-fix, fix-pass, post-fix — see the same File contract. Also add a test exercising the prepareFile branch where Fixer.RootDir is set, which the GitignoreFunc-wiring tests had been skipping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drop defensive error branches in new fix-path code User feedback: don't write error checks for branches that can't be reached. Mirror the pattern at internal/archetype/gensection/ranges.go:59 (\"NewFile never errors with current implementation\"). - buildPostFixFile: drop the lint.NewFile error return; signature becomes plain *lint.File. Caller no longer needs to handle a parse error after fix. - cachedGitignore: drop the filepath.Abs fallback. On the rare error case Abs returns the input string unchanged, which is still a usable cache key for the inputs the fix pipeline passes (filepath.Dir(path) or f.RootDir). Coverage on the new functions is now 100%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix cachedGitignore cache-key correctness Address PR #215 review (thread on internal/fix/fix.go:65): the previous "drop the Abs error fallback" simplification was wrong — filepath.Abs returns "" on failure (not the input string), so on the rare error path every relative dir would collide on the empty cache key and share one matcher across unrelated directories. My preceding comment also misdescribed Abs's error semantics. Fix the right way: don't normalize the cache key at all. lint.NewGitignoreMatcher does its own filepath.Abs internally to root the matcher, so the cache key only needs to be deterministic across calls within a Fix run, which prepareFile already guarantees by passing the same form (filepath.Dir(path) or f.RootDir). Use the dir string verbatim and update the comment to match what the code actually does. Add TestFixer_CachedGitignore_DistinctKeys documenting the cache contract: distinct inputs yield distinct matchers, repeated input hits the cache, empty-string input is its own entry rather than aliasing with everything else. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Make fix-pass probe actually exercise Fix Address PR #215 review (threads on test lines 258 and 309): the fixPassProbeRule was returning a diagnostic on Check call #1, which is the pre-fix engine.CheckRules pass — applyFixPasses then saw an empty diagnostic list on call #2 and never invoked Fix. So the \"validates hydration during applyFixPasses\" assertion was actually just validating hydration during pre/post-fix CheckRules, which the other tests in this file already cover. Rework: - Track Check and Fix snapshots separately. - Trigger the diagnostic on Check call #2 (the applyFixPasses pass) so Fix actually fires, then assert exactly 1 Fix snapshot. - Pin exact phase counts: 3 Check calls, 1 Fix call. - Assert the Fix call's lint.File also has the per-file context hydrated, not just the Check calls — that's the real regression guard for catalog/include rules whose Fix paths consult these fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Use filepath.Clean to normalize cachedGitignore key Address PR #215 review (thread on internal/fix/fix.go:64): the raw-key impl created separate cache entries for equivalent forms like "sub" vs "./sub" vs "sub/", undermining the cache when the caller didn't pre-normalize. Switch the cache key to filepath.Clean(dir): it's total (no error path, so no defensive fallback needed), idempotent, and collapses all the syntactic forms filepath.Clean considers equivalent. Don't use filepath.Abs (which is what Runner does for cross-cwd normalization) because Abs has a stdlib error contract that would require a defensive fallback. The Fixer's prepareFile passes the same form for all files in a Fix() call, so the only normalization that matters in practice is the syntactic-equivalence collapse that Clean already provides — and lint.NewGitignoreMatcher does its own filepath.Abs internally to root the matcher correctly. Extend TestFixer_CachedGitignore (now ...KeyContract) to pin the new equivalence-collapse contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "wipe staging %s" and "mkdir staging %s" error wraps added in the previous commit weren't reached by any existing test — codecov flagged them as new uncovered statements. Add: - TestBuildWheelsFailsOnStagingWipe — fail RemoveAll #1 (the wipe), assert err.Error() contains "wipe staging". - Tighten TestBuildWheelsFailsOnStagingMkdir to also assert err.Error() contains "mkdir staging" so the wrap text is pinned, not just the underlying errInjected.
* release: bump npm to >=11.5 + fail BuildWheels on empty-output build
Two failure modes from the v0.13.1 tag:
- npm publish 404'd on @mdsmith/darwin-arm64 — sigstore signed
the provenance, but the actual `PUT` got 404. Root cause:
Node 20 LTS ships npm 10.x, and npm Trusted Publishing
requires npm >= 11.5. Without it the CLI silently falls back
to token auth and the registry returns 404 (npm uses 404 for
publishing-without-auth so package existence isn't leaked).
Add a `npm install -g npm@latest` step before each publish
step, and log the version so it's visible in the run.
- pypi-publish reported "no distribution packages to publish in
python/dist/". `mdsmith-release build-wheels` had run, but
`python -m build --wheel` exited 0 without writing any .whl
to staging. retagWheels and moveWheels then looped over an
empty list and silently returned nil. Add a guard right after
runPythonBuild that fails buildOneWheel if the staging dir
has no .whl. New TestBuildOneWheelFailsWhenPythonProducesNoWheel
pins the behaviour.
Once both fixes ship, retag (e.g. v0.13.2) and the full
multi-channel publish should complete end-to-end.
* release: switch npm to Node 24 + fix relative-outdir wheel-build bug
Two follow-ups to the v0.13.1 fixes:
- Bump the npm-publish job's setup-node from "20" to "24". Node
24 ships npm 11.x natively (Trusted Publishing requires
>=11.5), so the install-g shim added in the previous commit
is unnecessary. Cleaner one-liner.
- Root cause for the v0.13.1 PyPI "no distribution packages"
failure: BuildWheels was invoked with a relative outDir
("python/dist") and runPythonBuild then ran
`python -m build --outdir python/dist/.staging-<plat>` with
cmd.Dir set to a staged temp tree. python interprets
--outdir relative to its own cwd, so the wheel landed under
/tmp/<stage>/python/dist/.staging-<plat>/. The Go side then
read <repo>/python/dist/.staging-<plat>/, found nothing, and
the empty-wheel guard fired. Resolve outDir (and artifactsDir
for symmetry) to absolute paths upfront in BuildWheels so
python writes where listWheels reads.
Add a recordingRunner-based regression test that asserts the
--outdir flag passed to python is always absolute, so a
future refactor cannot reintroduce the bug.
- Wipe `.staging-<plat>/` before running `python -m build`
(RemoveAll then MkdirAll). Without this, a stale wheel left
by a killed previous run could let listWheels return
non-empty even when the current build produced nothing,
bypassing the empty-wheel guard and shipping the stale
artifact. New TestBuildOneWheelWipesStaleStaging pins the
behaviour. Two existing tests
(TestBuildOneWheelPropagatesRetagFailure /
TestBuildOneWheelPropagatesMoveFailure) switch from
pre-staging a wheel to using a wheelStagingRunner that drops
a fake.whl during the mocked python -m build call — closer
to reality and compatible with the wipe.
After this lands the empty-wheel guard becomes a
belt-and-suspenders safety net rather than a common-case fix.
* release: absolute outDir + wipe stale staging in BuildWheels
Root cause for the v0.13.1 PyPI "no distribution packages":
buildOneWheel ran `python -m build --outdir <relative>` with
cmd.Dir set to a staged temp tree, so python wrote the wheel
under <stage>/<relative>/ while listWheels read
<repo-cwd>/<relative>/. Empty list, silent move-on, empty
python/dist at publish time.
Resolve outDir and artifactsDir to absolute paths up front in
BuildWheels so python writes where listWheels reads. Also wipe
.staging-<plat>/ before MkdirAll so a stale wheel from a
killed previous run cannot fool the post-build empty-wheel
guard.
The companion test changes are pushed in a separate commit.
* release: regression tests for absolute --outdir + stale-staging wipe
Three new fault-injection tests:
- TestBuildWheelsPassesAbsoluteOutdirToPython — recordingRunner
asserts the --outdir flag passed to python -m build is always
absolute, so the v0.13.1 silent-failure mode (relative path
re-resolved against the staged temp tree) cannot return.
- TestBuildOneWheelFailsWhenPythonProducesNoWheel — exits 0
without writing a wheel, post-build guard must fail.
- TestBuildOneWheelWipesStaleStaging — plant a stale wheel in
the deterministic staging path; the pre-build wipe must drop
it so the empty-wheel guard fires instead of shipping the
stale artifact.
Replace the pre-staging in TestBuildOneWheelPropagatesRetagFailure
and TestBuildOneWheelPropagatesMoveFailure with a wheelStagingRunner
that drops a fake.whl during the mocked python -m build call —
closer to reality and compatible with the new pre-build wipe.
* release: wrap staging-dir errors with path context
Per Copilot review: bare `return err` on RemoveAll/MkdirAll
of the staging dir made release-time failures hard to
diagnose. Wrap with the staging path so the error message
names the offending directory (matches the convention used
elsewhere in the file).
* test: surface wheelStagingRunner WriteFile errors
Per Copilot review: wheelStagingRunner ignored the error from
os.WriteFile when staging the fake .whl. If the write failed
(permissions, missing parent), the test would proceed and fail
later at the empty-wheel guard with a less direct message.
Return the wrapped write error so the failure points at the
real cause.
* release: assert npm >= 11.5 before publishing
Defensive guardrail per Copilot review: even though Node 24
currently ships npm 11.x, a future Node 24 patch could bundle
an older CLI. npm Trusted Publishing requires >= 11.5; without
it the publish silently 404s.
Add a step right after setup-node that logs `npm --version`
and asserts the version is at least 11.5.0 via a small node
-e check, exiting with a clear message otherwise.
* test: cover BuildWheels staging-wipe and mkdir error wraps
The "wipe staging %s" and "mkdir staging %s" error wraps added
in the previous commit weren't reached by any existing test —
codecov flagged them as new uncovered statements. Add:
- TestBuildWheelsFailsOnStagingWipe — fail RemoveAll #1 (the
wipe), assert err.Error() contains "wipe staging".
- Tighten TestBuildWheelsFailsOnStagingMkdir to also assert
err.Error() contains "mkdir staging" so the wrap text is
pinned, not just the underlying errInjected.
* test: cover BuildWheels listWheels + filepath.Abs error branches
codecov/patch flagged three new statements as uncovered:
- buildOneWheel's listWheels error path between runPythonBuild
and the empty-wheel guard
- BuildWheels' two filepath.Abs error wraps (resolve outDir /
resolve artifactsDir)
filepath.Abs only fails when os.Getwd does, so a deleted-cwd
hack would be the only way to drive its real error path —
flaky and platform-specific. Add a package-level absPath seam
that aliases filepath.Abs in production and lets tests swap in
a stub returning errInjected. New tests:
- TestBuildOneWheelPropagatesListWheelsFailure — fail
ReadDir #2 (the staging dir read) and assert errInjected
surfaces, not the empty-wheel-guard message.
- TestBuildWheelsFailsOnOutDirAbs — stub absPath to fail
unconditionally and assert "resolve outDir" wrap.
- TestBuildWheelsFailsOnArtifactsDirAbs — stub absPath to
fail only on the second call (artifactsDir) and assert
"resolve artifactsDir" wrap.
Local coverage on internal/release: 98.3% -> 99.6%. Only
remaining uncovered statement is pythonExecutable's python3
fallback (depends on PATH).
https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6
---------
Co-authored-by: Claude <noreply@anthropic.com>
Two header-detection bugs surfaced by Copilot review: - `isHeader` rejected any line whose trimmed content started with `#`, but `#1 | Title` is a valid first cell, not an ATX heading. Limit the guard to actual ATX shape — one to six `#` followed by space, tab, or end of line — via a new `isATXHeading` helper. - `parseRow` checked `HasPrefix(c, "|")` against the un-trimmed row content, so a row like `> | a | b |` (extra indent after the blockquote marker) had `leading` come out false even though `logicalCells` already trimmed and treated it as a real edge. Trim the same way before edge detection. Addresses Copilot review feedback on PR #353. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT
Two header-detection bugs surfaced by Copilot review: - `isHeader` rejected any line whose trimmed content started with `#`, but `#1 | Title` is a valid first cell, not an ATX heading. Limit the guard to actual ATX shape — one to six `#` followed by space, tab, or end of line — via a new `isATXHeading` helper. - `parseRow` checked `HasPrefix(c, "|")` against the un-trimmed row content, so a row like `> | a | b |` (extra indent after the blockquote marker) had `leading` come out false even though `logicalCells` already trimmed and treated it as a real edge. Trim the same way before edge detection. Addresses Copilot review feedback on PR #353. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT
Two header-detection bugs surfaced by Copilot review: - `isHeader` rejected any line whose trimmed content started with `#`, but `#1 | Title` is a valid first cell, not an ATX heading. Limit the guard to actual ATX shape — one to six `#` followed by space, tab, or end of line — via a new `isATXHeading` helper. - `parseRow` checked `HasPrefix(c, "|")` against the un-trimmed row content, so a row like `> | a | b |` (extra indent after the blockquote marker) had `leading` come out false even though `logicalCells` already trimmed and treated it as a real edge. Trim the same way before edge detection. Addresses Copilot review feedback on PR #353. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT
Two header-detection bugs surfaced by Copilot review: - `isHeader` rejected any line whose trimmed content started with `#`, but `#1 | Title` is a valid first cell, not an ATX heading. Limit the guard to actual ATX shape — one to six `#` followed by space, tab, or end of line — via a new `isATXHeading` helper. - `parseRow` checked `HasPrefix(c, "|")` against the un-trimmed row content, so a row like `> | a | b |` (extra indent after the blockquote marker) had `leading` come out false even though `logicalCells` already trimmed and treated it as a real edge. Trim the same way before edge detection. Addresses Copilot review feedback on PR #353. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT
* Add MDS060 table-structure rule (plan 181) New default-enabled rule covering markdownlint MD055 (table-pipe-style), MD056 (table-column-count), and MD058 (blanks-around-tables). MD055 and MD058 are autofixed; MD056 is flagged only since a missing cell's content is unknown. Uses line-based GFM table detection (edge pipes optional) so it sees the borderless and mixed-pipe tables MDS025's tablefmt parser cannot. The default `consistent` style is loop-stable with MDS025 enabled, since MDS025's canonical bordered output already satisfies it. Generated catalog/include table bodies are skipped so the source file stays the owner. Closes the MD055/MD056/MD058 gap in the linter comparison. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * MDS060: insert CRLF-matching blank lines in fix The MD058 blank-line insertion emitted a bare "" line, which on a CRLF file produced a lone-LF blank among CRLF rows (mixed endings). Detect the file's newline style and insert a matching blank line, mirroring the edge-normalization path that already preserves trailing carriage returns. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * MDS060: lint blockquoted and indented tables Extend prefix detection to consume a `>` blockquote-marker chain (mirroring MDS025's tablefmt), so MD055/MD056/MD058 apply to blockquoted and list-indented tables instead of being silently skipped. The MD058 blank line inside a blockquote is the bare `>` marker, not an empty line, so the blockquote is not broken; CRLF newline style is preserved. Adds blockquote fixtures and brings the package to 100% statement coverage (addresses the codecov/patch gate). Addresses Copilot review feedback on PR #353. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * MDS060: handle escaped trailing pipe; correct MDS025 note Edge detection treated a literal `\|` at the end of the last cell as a trailing edge pipe, producing a false MD055/MD056 diagnostic and corrupting the cell on fix. Trailing-pipe detection, cell counting, and edge normalization now strip a final `|` only when it is unescaped (even backslash run). Also correct the README guidance: no_leading_or_trailing does not oscillate with MDS025. Once MDS060 strips the edges, MDS025 (bordered-only) stops formatting the table; the real tradeoff is lost column alignment, not a per-pass disagreement. Backed by a loop-stability test. Addresses Copilot review feedback on PR #353. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * MDS060: respect backslash parity for unescaped pipes Three call sites treated every literal `|` as a delimiter regardless of escaping. A paragraph like `A \| B` could be mistaken for a table header; a paragraph after a table whose only pipe was escaped was absorbed as a body row (hiding the MD058 "missing blank line after" diagnostic); and `splitCells` treated `\\|` as a single literal pipe rather than an escaped backslash followed by a real delimiter, miscounting cells for MD056. Introduce `containsUnescapedPipe` (used in `isHeader`, `isSeparator`, `continuesTable`) and rewrite `splitCells` to toggle an escape state byte by byte. Cell counts and table boundaries now match the same backslash-parity rule as `endsWithUnescapedPipe`. Addresses Copilot review feedback on PR #353. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * MDS060: declare markdownlint frontmatter rules The rule-readme schema (internal/rules/proto.md) now requires each README to list the markdownlint rule(s) it covers, so MDS020 can validate the coverage matrix from front matter instead of free-form prose. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * MDS060: tighten ATX-heading guard and post-prefix indent Two header-detection bugs surfaced by Copilot review: - `isHeader` rejected any line whose trimmed content started with `#`, but `#1 | Title` is a valid first cell, not an ATX heading. Limit the guard to actual ATX shape — one to six `#` followed by space, tab, or end of line — via a new `isATXHeading` helper. - `parseRow` checked `HasPrefix(c, "|")` against the un-trimmed row content, so a row like `> | a | b |` (extra indent after the blockquote marker) had `leading` come out false even though `logicalCells` already trimmed and treated it as a real edge. Trim the same way before edge detection. Addresses Copilot review feedback on PR #353. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * fold table structure into MDS025 table-format Merge MDS060 (table-structure) into MDS025 so a single rule owns GFM table parsing, the structure checks (MD055 pipe style, MD056 column count, MD058 blanks-around-tables), and the prettier-style alignment pass. Users now configure one `table-format` block; a `mdsmith fix` run is inherently single-pass with no inter-rule oscillation window. - Port the GFM parser and MD055/056/058 logic into internal/rules/tableformat/structure.go. - Extend tableformat.Rule with a `style` setting (consistent / leading_and_trailing / no_leading_or_trailing) and chain the structure fix before the alignment fix on the same Fix call. - Bring the alignment pass's skip set to parity with the structure pass via formatSkipLines (code blocks + PI blocks + generated ranges). The helper builds a fresh map; lint.Collect*BlockLines return a shared read-only cache. - Migrate the structure fixtures into internal/rules/MDS025-table-format/{good,bad,fixed}/ with merged-rule diagnostic lists. The short-row fixture now expects both a format diag and the MD056 structure diag; the alignment pass pads the short cell on Fix. - Delete the tablestructure package and MDS060 fixture dir; drop the registrations in internal/rules/all and the integration test. - Update the MDS025 README, the markdownlint-coverage research doc, and the linter-comparison background page for the merged scope. - Refresh plan/181_table-structure.md to describe the merged design and tasks. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * recompute GeneratedRanges after structure fix The structure pass inserts blank lines around tables that need MD058 fix. Those insertions shift every downstream generated section by N lines. Fix previously copied f.GeneratedRanges onto the reparsed buffer, so the alignment pass's skip set pointed at pre-fix line numbers; tablefmt would then iterate past the shifted body and rewrite a non-canonical table living inside an `<?include?>` or `<?catalog?>` directive. Call gensection.FindAllGeneratedRanges on the reparsed buffer instead. The new TestFix_RecomputesGeneratedRangesAfterStructure Insert reproduces the bug (verified by temporarily reverting to the copy — the body table got reformatted) and gates the fix. Also rephrase the plan: "inherently single-pass" was imprecise. The fix engine still loops fixable rules to stability; the new wording says one Rule.Fix call runs structure + alignment, and that MDS025 has no second rule to oscillate against. The package-comment "retired MDS060" is dropped in favor of naming the markdownlint coverage directly. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * post-rebase fixups: gofmt and README include regen After rebasing onto main, gofmt collapses the Pad field's comment alignment and `mdsmith fix` updates the README's `<?include?>` bodies to pick up the spaced separators main introduced in good/default.md and good/alignment.md. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * address copilot review: sort, dedupe, CRLF, escape, skip cache Five correctness/consistency fixes from Copilot's second pass: 1. Check sorts the combined diagnostic slice by (line, column) after appending structure diagnostics to the format-pass output. With multiple tables in one file the two streams interleave by line, and fixture tests compare diagnostics in source order. 2. applyStructureFix dedupes adjacent-table MD058 insertions: two tables with different prefixes can each schedule a blank at the same gap (table1's blankAfter[K] + table2's blankBefore[K+1]). Emitting both produces consecutive blank lines that then trip MDS008 no-multiple-blanks. Test: TestMD058_NoDoubleBlankBetweenAdjacentTables. 3. Fix re-normalises line endings to CRLF when the source used it. tablefmt joins rewritten table lines with bare `\n`, dropping each row's `\r`, which on CRLF documents leaves the table with bare-LF endings while every surrounding line keeps `\r\n` — a mixed-ending output. Test: TestFix_CRLF_RoundTrips_TableLines. 4. Structure escape semantics drop backslash parity and match tablefmt's GFM rule directly: `\|` is the only escape, so `\\|` reads as a literal backslash plus an escaped pipe (one cell), not "escaped backslash + unescaped delimiter" (two cells). The earlier parity behavior put the structure pass and tablefmt at odds on inputs containing `\\|`. Updated tests: TestSplitCells_EscapedPipe, TestEndsWithUnescapedPipe, TestContainsUnescapedPipe. 5. formatSkipLines returns the cached code-block map directly when there are no PI blocks and no generated ranges, avoiding a per-Check allocation on the hot path. The merged map is built only when one of the other inputs is non-empty. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * markdownlint-coverage: use legend's "partial" for MD056 The coverage matrix used a one-off "⚠️ " status marker for MD056 that the file's status legend (✅ / partial / 🔲 plan N) does not define. Switch to the existing "partial" label so the table is self-consistent. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * docs: qualify the MD056 "alignment pads short rows" claim Both the rule README and plan 181 said a fixed file is structurally clean even when a cell is missing, on the assumption that the alignment pass would pad the short row. That only holds for *bordered* tables: tablefmt requires edge pipes on every row, so a borderless short row survives the fix untouched and MD056 keeps firing. Qualify the claim and tell the user how to resolve the borderless case (add edges or fill the cell). https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * docs: MD056 covers the delimiter row too The README said only "body row" cell counts are compared, but the implementation iterates `t.rows[1:]` so the delimiter row is also matched against the header. Catching a delimiter row with the wrong cell count is intentional — a malformed delimiter would otherwise pass MD056 silently. Reword the spec line to match the behaviour. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * structure: reject bare-pipe lines as table headers isHeader accepted a single `|` (no logical cell) as a valid header because it only checked for an unescaped pipe and a non-separator shape. That produces false-positive table detection — e.g. `|` followed by a delimiter-looking line — and diverges from tablefmt, which requires a row to start/end with `|` and have length >= 2. Require `countCells(c) > 0` before accepting the line as a header. Guarded by TestBarePipeNotHeader (red without the guard). https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * perf: stream structure fix + push CRLF into tablefmt Two perf fixes for Copilot's second-pass review: - applyStructureFix used to materialise every line as a string, modify the slice, then rebuild via strings.Join. On a typical file that's N+5 allocations even when the file has no tables to rewrite. Replace with a bytes.Buffer pre-sized to the source, writing untouched rows directly from f.Lines as []byte and only string-converting the rows that actually need edge-normalisation. Split into collectStructureEdits + renderStructureFix to keep each function under the gocognit budget. - Rule.Fix used to scan the whole output buffer twice (`bytes.ReplaceAll(\r\n -> \n)` then `\n -> \r\n`) so the CRLF endings tablefmt stripped from rewritten table rows came back. Push CRLF awareness into tablefmt.rebuildWithFormattedTables instead: if any source line ends with `\r`, re-append it to each formatted row. Drop the post-pass from Fix. The existing CRLF round-trip test and adjacent-tables dedupe test still pass. https://claude.ai/code/session_012X1wVbY7u9DNMpGRhNurzT * post-rebase: regen rule catalog to include MDS067 origin/main added MDS067 (callout-type) while this branch was open. The rebase took the local catalog body during conflict resolution; re-running `mdsmith fix` rebuilds the row. --------- Co-authored-by: Claude <noreply@anthropic.com>
Addresses 15 issues from the multi-angle code review. The previous
regex scanner had real correctness gaps and ergonomic limitations.
Bugs the old scanner had:
1. Brace inside string literals (e.g. `{{ printf "{%s}" .Params.summary }}`)
silently skipped the action — the naive `{{[^{}]*}}` regex stopped
at the inner `{`.
2. Positional renderSummary regex matched when `.Params.summary` was
nested in a non-RenderString call: `{{ .RenderString (printf "%s"
.Params.summary) }}` falsely passed.
3. Piped renderSummary regex matched any later `.RenderString`
mention: `{{ .Params.summary | print "x" .Page.RenderString }}`
falsely passed because `.RenderString` appears after the pipe.
4. ifPredicate anchored to exact `if .Params.summary`, so compound
forms (`if and .Params.summary $cond`) and `else if` were flagged
as violations even though they only check presence.
5. Variable assignment `{{ $s := .Params.summary }}` was flagged
indiscriminately; subfield access `{{ if .Params.summary.X }}`
was flagged because the predicate regex required exact end.
6. Hugo comments mentioning the field (`{{/* .Params.summary */}}`)
were treated as live references.
The new scanner:
- Tokenizes actions with quote-aware lexing (handles double-quoted
and backtick strings; `{` and `}` inside strings no longer split
actions) — fixes #1.
- Strips `{{/* ... */}}` comments before scanning (preserves line
numbers via newline padding) — fixes #6.
- Classifies each action by leading keyword. `if` / `else if` /
`range` are presence predicates regardless of compound form or
subfield access — fixes #4 and the subfield case. `with` /
`else with` are flagged as rebinding the dot.
- Pipeline analysis: splits on `|` at paren-depth 0, walks each
stage. `.Params.summary` is safe only when it is a top-level
positional argument to `.RenderString` or the head of a pipeline
whose terminal stage is `.RenderString`. Nested references inside
parens are flagged — fixes #2 and #3.
Other fixes in this commit:
- `baseof.html` exemption is now by relative path
(`_default/baseof.html`), not basename. Hugo idiomatically
supports per-type baseof overrides; basename-only would silently
exempt all of them.
- The walker collects I/O errors into a side slice and continues,
so a transient ReadFile failure on one file no longer masks
every violation in the remaining files.
- `layoutsPath` deleted; the walk calls the existing `repoRoot(t)`
helper from messaging_test.go.
- Three duplicated regex blocks collapsed into package-level vars
and a single `scanSummaryViolations` helper. The multi-line test
no longer round-trips through a tempfile.
- baseof.html's meta description pipes the summary through
`$.RenderString (dict "display" "inline") . | plainify` so
backticks become `<code>` HTML and then plain text. SEO
snippets ship clean prose instead of literal Markdown
punctuation.
- The pipe-form alternation (YAGNI in the previous regex) is
generalized by the pipeline walker; no current template uses
the piped form but the scanner handles it correctly when one
does.
Verified by: 19-case `TestClassifyAction_TableDriven` covering
every safe and unsafe shape, plus three scenario tests
(`TestFindActions_BalancedStrings`,
`TestScanSummaryViolations_CommentsIgnored`,
`TestScanSummaryViolations_MultiLineWith`,
`TestScanSummaryViolations_BraceInString`). The real-layout walk
(`TestSummaryFrontMatterRenderedThroughRenderString`) passes
against the current `website/layouts/` tree, and the rendered
meta description on the progressive-disclosure page now shows
"Use <?catalog?> ..." rather than "Use \`<?catalog?>\` ...".
docs/development/website-config.md updated to enumerate the safe
and forbidden forms and explain the baseof.html meta-description
plainify path.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
Acts on every concrete finding from the local max-effort review pass except #5 (linkRefResetter interface duplication), which I'm keeping local: exposing it from pkg/markdown would surface a goldmark fork detail on the public surface for less duplication value than it costs. Bugs - fix.go (#1): MDS034 Fix was calling flavor.NewPooledParser() per invocation — same regression I fixed in flavor.Detect last commit, mirrored at the parallel Fix call site. Move the pool to package level in flavor and expose a callback API, flavor.WithSharedParser, used by both Detect's dualFindings and the rule's fixByteRangeFeatures. - rule.go fixGitHubAlerts (#7): the type assertion bq.FirstChild().(*ast.Paragraph) plus lines.At(0) was relying on a cross-package contract with flavor.IsGitHubAlert. Re-check the shape locally so a future relax of IsGitHubAlert cannot turn the walk into a panic. - fix.go taskCheckBoxEdits (#9): nil-check the flavor.NearestBlockAncestor return and the block's Lines() before calling At(0). - detect.go (#11, #15): IsGitHubAlert nil-guards bq and the paragraph's Lines.Len(); findHeadingID nil-guards h. Both are public-API entry points now. - rule.go ApplySettings (#12): iterate settings keys in sorted order so the error for multiple unknown settings is deterministic across Go map randomisation. Simplifications - detect.go (#2): drop the taskCheckBoxFinding specialisation; the TaskCheckBox case in builtinFindingFor calls inlineExtFinding directly. - detect.go (#3): promote isGitHubAlert / lineCol / nearestBlockAncestor to the exported names. The previous private + one-line public-wrapper pair was duplication; the lowercase versions are now gone. - detect.go dualFindings (#4): return nil rather than an always-allocated empty slice on no findings (CLAUDE.md allocation-budget rule). - parser.go (#6): drop NewParser and NewParserWith — they were one-line wrappers around the pooled forms. The public surface is now NewPooledParser, NewPooledParserWith, and WithSharedParser. - contract_test.go (#13): move signature pins to package-scope `var _ = ...` declarations so the staticcheck "could omit type" rule does not fight the explicit-type contract. Reuse - pkg/markdown.Edit + Splice (#8): added an optional `Repl []byte` field to Edit; Splice now supports replacement in addition to deletion. The rule's bespoke `edit` struct and `applyEdits` are gone; fix.go composes a []markdown.Edit and feeds it through markdown.Splice. Adjacent-edits-with-Repl behaviour is now pinned in pkg/markdown's TestSplice. - lint.UnmarshalFrontMatter (#14): extracted the StripFrontMatter → trim `---\n` delimiters → yamlutil.UnmarshalSafe pipeline into one helper in internal/lint/frontmatter.go. internal/integration's rules_test.go switched to it, dropping its open-coded copy and the goldmark-frontmatter import. Test pyramid - Added unit tests for IsGitHubAlert's nil/empty-Lines branches, FindHeadingID's nil-heading branch, and TestWithSharedParser for the new pool callback. Coverage in pkg/markdown/flavor is 100%. - TestApplyEditsHandlesAdjacentEdits moved into pkg/markdown's TestSplice as a sub-test covering the new Repl behaviour. All tests pass, golangci-lint clean, mdsmith check clean. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
… render, docs
Multiple findings from the angles:
A1 — ChainNode boundary case (`(.Params).summary`): the
`Params`/`summary` adjacency straddles the receiver/Field
boundary. Neither half alone has the pair. Added tailIdents
to flatten a chain receiver into one Ident slice and check
for the pair across the boundary. Caught: bare ChainNode,
nested ChainNode `((.A).Params).summary`, and function-call
receiver via fallback recurse.
A2 — `{{ $s := .RenderString .Params.summary }}` was being
flagged as variable assignment of the raw summary, but the
RHS routes summary through RenderString so the bound name
holds template.HTML (rendered Markdown). Hugo emits
template.HTML without re-escaping, so a later `{{ $s }}`
ships rendered output. pipeAssignsSummary now skips the
flag when the RHS already outputs via RenderString.
Sweep #1 — Forbidden-forms list in docs was missing the
TemplateNode case (`{{ template "name" .Params.summary }}`
and the `{{ block }}` shorthand). Added.
Sweep #2 — Vacuous-pass guard was `scanned >= 5` but the
tree has 24 .html files. Raised to 20.
Sweep #3 — `assert.Empty(t, formatted)` had no diagnostic
message; sibling `ioErrors` line did. Added.
Sweep #4 / D1 — Commented the deliberate case-sensitivity
asymmetry: identsReferenceSummary uses EqualFold (Hugo
Params map is case-insensitive); cmdIsRenderString uses
`==` (Go method names are case-sensitive). Without the
comment a maintainer "normalising" one would silently
break the rule.
Sweep #7 / D6 — Commented the inner `if n == nil` guard
inside the *parse.ListNode case. The typed-nil ElseList
of an if-without-else bypasses the outer guard; removing
the inner check would re-introduce a panic.
tailIdents — pruned the unreachable VariableNode case (no
template syntax produces a ChainNode whose receiver is a
bare VariableNode; `$s.Field` parses as VariableNode with
the field appended to its own Ident). Per CLAUDE.md, no
defensive branch without a red/green driver.
Coverage: templatecheck package now at 100.0% of statements.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
Acts on every concrete finding from the local max-effort review pass except #5 (linkRefResetter interface duplication), which I'm keeping local: exposing it from pkg/markdown would surface a goldmark fork detail on the public surface for less duplication value than it costs. Bugs - fix.go (#1): MDS034 Fix was calling flavor.NewPooledParser() per invocation — same regression I fixed in flavor.Detect last commit, mirrored at the parallel Fix call site. Move the pool to package level in flavor and expose a callback API, flavor.WithSharedParser, used by both Detect's dualFindings and the rule's fixByteRangeFeatures. - rule.go fixGitHubAlerts (#7): the type assertion bq.FirstChild().(*ast.Paragraph) plus lines.At(0) was relying on a cross-package contract with flavor.IsGitHubAlert. Re-check the shape locally so a future relax of IsGitHubAlert cannot turn the walk into a panic. - fix.go taskCheckBoxEdits (#9): nil-check the flavor.NearestBlockAncestor return and the block's Lines() before calling At(0). - detect.go (#11, #15): IsGitHubAlert nil-guards bq and the paragraph's Lines.Len(); findHeadingID nil-guards h. Both are public-API entry points now. - rule.go ApplySettings (#12): iterate settings keys in sorted order so the error for multiple unknown settings is deterministic across Go map randomisation. Simplifications - detect.go (#2): drop the taskCheckBoxFinding specialisation; the TaskCheckBox case in builtinFindingFor calls inlineExtFinding directly. - detect.go (#3): promote isGitHubAlert / lineCol / nearestBlockAncestor to the exported names. The previous private + one-line public-wrapper pair was duplication; the lowercase versions are now gone. - detect.go dualFindings (#4): return nil rather than an always-allocated empty slice on no findings (CLAUDE.md allocation-budget rule). - parser.go (#6): drop NewParser and NewParserWith — they were one-line wrappers around the pooled forms. The public surface is now NewPooledParser, NewPooledParserWith, and WithSharedParser. - contract_test.go (#13): move signature pins to package-scope `var _ = ...` declarations so the staticcheck "could omit type" rule does not fight the explicit-type contract. Reuse - pkg/markdown.Edit + Splice (#8): added an optional `Repl []byte` field to Edit; Splice now supports replacement in addition to deletion. The rule's bespoke `edit` struct and `applyEdits` are gone; fix.go composes a []markdown.Edit and feeds it through markdown.Splice. Adjacent-edits-with-Repl behaviour is now pinned in pkg/markdown's TestSplice. - lint.UnmarshalFrontMatter (#14): extracted the StripFrontMatter → trim `---\n` delimiters → yamlutil.UnmarshalSafe pipeline into one helper in internal/lint/frontmatter.go. internal/integration's rules_test.go switched to it, dropping its open-coded copy and the goldmark-frontmatter import. Test pyramid - Added unit tests for IsGitHubAlert's nil/empty-Lines branches, FindHeadingID's nil-heading branch, and TestWithSharedParser for the new pool callback. Coverage in pkg/markdown/flavor is 100%. - TestApplyEditsHandlesAdjacentEdits moved into pkg/markdown's TestSplice as a sub-test covering the new Repl behaviour. All tests pass, golangci-lint clean, mdsmith check clean. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
…markdown (#409) * Start plan 185: Expose extended-syntax parsers and the flavor model in pkg/markdown * Plan 185: expose extended-syntax parsers and flavor model in pkg/markdown Promotes every custom goldmark parser and the per-flavor Feature support model into a new public pkg/markdown/flavor sub-package. The MDS034 rule is reduced to a thin adapter; internal/schema's hand-rolled goldmark config and the rule's own dual parser both fold into the single goldmark.New call site in pkg/markdown/flavor/parser.go. - pkg/markdown/flavor: Flavor and Feature types, the support table, Detect(doc *markdown.Document, accept func(Feature) bool) []Finding, Finding/HeadingIDExtra shapes, four NewParser*/NewPooledParser* constructors, and small rewriter helpers (FindHeadingID, IsGitHubAlert, LineCol). - pkg/markdown/flavor/ext: the five custom extensions (Superscript, Subscript, MathBlock, MathInline, Abbreviation) and their AST node kinds, moved wholesale from internal/rules/markdownflavor/ext. - internal/convention: Flavor is now a type alias for pkg/markdown/flavor.Flavor; constants and ParseFlavor are re-exported so internal/config compiles unchanged. - internal/rules/markdownflavor: Check builds a *markdown.Document from *lint.File, calls flavor.Detect, and maps findings to diagnostics. Fix retains its byte-range edit pipeline but uses flavor.NewPooledParser instead of a private singleton. - internal/schema/validate_content.go: replaces the local goldmark.New(extension.Table) with flavor.NewPooledParserWith. - internal/integration/rules_test.go: drops the goldmark-frontmatter test helper in favour of lint.StripFrontMatter + yamlutil.UnmarshalSafe so no goldmark.New remains under internal/. - Contract test pins the public flavor API shape. - Docs (markdown-library.md, architecture/index.md, cross-system.md, go.md) updated to drop the "CommonMark only" wording and document the new sub-package. Verifies all acceptance criteria: pkg/markdown imports no internal/, no goldmark.New under internal/ or cmd/, no custom AST node types or parsers outside pkg/markdown, and MDS034 / schema diagnostics remain byte-identical (existing tests pass unchanged). https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Fix broken links to moved ext package and lift FindHeadingID coverage The pinned-version and source mdsmith-check jobs both failed: three plan/185 references and one architecture-audit reference still pointed at the now-empty internal/rules/markdownflavor/ext path. Locally the references resolved because git mv left an empty directory behind; CI's fresh checkout sees them as dead links. - plan/185_public-markdown-flavor-library.md: retarget every internal/rules/markdownflavor/ext link at pkg/markdown/flavor/ext (the new home). - docs/development/architecture-audit.md: rewrite the "markdownflavor/ext sub-package" finding as resolved by plan/185, drop the broken link, and trim to stay under the 300-line cap. - pkg/markdown/flavor/detect.go: drop the defensive nil-AST guards I added in detectBareURLs / detectGitHubAlerts; flavor.Detect already short-circuits on doc == nil and the original *lint.File variant carried no such check. - pkg/markdown/flavor/detect_edge_test.go: exercise both branches of the public FindHeadingID wrapper so codecov/patch sees full coverage on the new helper. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Fix duplicate link-ref transformer in newParserInternal Copilot review caught a real bug: goldmark.New() calls DefaultParser() which already installs DefaultParagraphTransformers(), so the previous goldmark.WithParserOptions(parser.WithParagraphTransformers(defaults...)) call appended a second link-reference transformer on top. The reset closure only touched the appended instance; the one inside the default parser kept pinning the last parsed document's bytes. Build the parser explicitly with parser.NewParser (one set of block, inline, and paragraph parsers including the lrp captured for reset) and install it via goldmark.WithParser, then let goldmark.New's extension Extend hooks register the additional block / inline parsers they need. After this change there is exactly one link-ref transformer in the resulting parser, and the closure returned to NewPooledParser callers resets that instance. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Pool the dual parser in flavor.Detect; inline onlyAccept Self-review found two issues from the plan-185 changes: 1. Detect built a fresh goldmark parser per call via NewPooledParser, running the full Extend hook chain on every Check. The previous singleton in internal/rules/markdownflavor avoided this; the move to a stateless public Detect regressed it. Add a sync.Pool inside the flavor package that hands each Detect goroutine its own parser-with-reset pair, mirroring internal/schema's contentParserPool. The pool resets the link-reference transformer before Put so idle slots do not pin document bytes. 2. fix.go used a one-line onlyAccept helper that was only ever called from one site. Inline the closure literal at the call site and drop the helper. A new BenchmarkDetectReusesPool exercises the dual-parser code path repeatedly; coverage in pkg/markdown/flavor stays at 100%. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Expose NearestBlockAncestor, add byte-identical pin test, fill pyramid Addresses the two remaining self-review concerns and aligns the package's tests with the test-pyramid rule that every production function ships its dedicated unit test by name. Concern #2 — drop the nearestBlockAncestor duplicate: - Expose NearestBlockAncestor from pkg/markdown/flavor so external rewriters (and the rule adapter) share the helper instead of duplicating it. Replace the rule's private copy in fix.go with flavor.NearestBlockAncestor. - Add it to the contract test, the markdown-library stable surface list, and a dedicated TestNearestBlockAncestorPublic test. Concern #3 — byte-identical pin test: - pkg/markdown/flavor/detect_pin_test.go adds a corpus-driven table test (pinCorpus) that maps each input to the exact Finding stream (feature + 1-based line + 1-based column, in document order). Plan 185 acceptance criterion "Table tests pin this" is now an explicit gate; any subtle reorder, drop, or shift in MDS034 diagnostics will break the test with a side-by-side diff. Test-pyramid alignment: - TestNearestBlockAncestor (subtests for the skip-non-block and orphan branches) plus TestNearestBlockAncestorPublic for the exported wrapper. - TestIsGitHubAlertPublic exercises both branches of IsGitHubAlert (alert blockquote / heading-first-child). - TestLineColPublic pins the documented 1-based semantics of the exported LineCol wrapper. - TestDualFindings covers the dualFindings helper I extracted from Detect in the previous commit, asserting both the keep-filter and the still-emits-other-features path. Coverage in pkg/markdown/flavor stays at 100%; mdsmith check and golangci-lint are clean. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Address recall-mode review findings (14 of 15) Acts on every concrete finding from the local max-effort review pass except #5 (linkRefResetter interface duplication), which I'm keeping local: exposing it from pkg/markdown would surface a goldmark fork detail on the public surface for less duplication value than it costs. Bugs - fix.go (#1): MDS034 Fix was calling flavor.NewPooledParser() per invocation — same regression I fixed in flavor.Detect last commit, mirrored at the parallel Fix call site. Move the pool to package level in flavor and expose a callback API, flavor.WithSharedParser, used by both Detect's dualFindings and the rule's fixByteRangeFeatures. - rule.go fixGitHubAlerts (#7): the type assertion bq.FirstChild().(*ast.Paragraph) plus lines.At(0) was relying on a cross-package contract with flavor.IsGitHubAlert. Re-check the shape locally so a future relax of IsGitHubAlert cannot turn the walk into a panic. - fix.go taskCheckBoxEdits (#9): nil-check the flavor.NearestBlockAncestor return and the block's Lines() before calling At(0). - detect.go (#11, #15): IsGitHubAlert nil-guards bq and the paragraph's Lines.Len(); findHeadingID nil-guards h. Both are public-API entry points now. - rule.go ApplySettings (#12): iterate settings keys in sorted order so the error for multiple unknown settings is deterministic across Go map randomisation. Simplifications - detect.go (#2): drop the taskCheckBoxFinding specialisation; the TaskCheckBox case in builtinFindingFor calls inlineExtFinding directly. - detect.go (#3): promote isGitHubAlert / lineCol / nearestBlockAncestor to the exported names. The previous private + one-line public-wrapper pair was duplication; the lowercase versions are now gone. - detect.go dualFindings (#4): return nil rather than an always-allocated empty slice on no findings (CLAUDE.md allocation-budget rule). - parser.go (#6): drop NewParser and NewParserWith — they were one-line wrappers around the pooled forms. The public surface is now NewPooledParser, NewPooledParserWith, and WithSharedParser. - contract_test.go (#13): move signature pins to package-scope `var _ = ...` declarations so the staticcheck "could omit type" rule does not fight the explicit-type contract. Reuse - pkg/markdown.Edit + Splice (#8): added an optional `Repl []byte` field to Edit; Splice now supports replacement in addition to deletion. The rule's bespoke `edit` struct and `applyEdits` are gone; fix.go composes a []markdown.Edit and feeds it through markdown.Splice. Adjacent-edits-with-Repl behaviour is now pinned in pkg/markdown's TestSplice. - lint.UnmarshalFrontMatter (#14): extracted the StripFrontMatter → trim `---\n` delimiters → yamlutil.UnmarshalSafe pipeline into one helper in internal/lint/frontmatter.go. internal/integration's rules_test.go switched to it, dropping its open-coded copy and the goldmark-frontmatter import. Test pyramid - Added unit tests for IsGitHubAlert's nil/empty-Lines branches, FindHeadingID's nil-heading branch, and TestWithSharedParser for the new pool callback. Coverage in pkg/markdown/flavor is 100%. - TestApplyEditsHandlesAdjacentEdits moved into pkg/markdown's TestSplice as a sub-test covering the new Repl behaviour. All tests pass, golangci-lint clean, mdsmith check clean. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Refresh bench comment after pool move The pool moved from inside Detect to package level in commit 08446eb (WithSharedParser). Update the bench's docstring to point at the new home so the next reader does not look in the wrong file. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Address second-round recall review (F1, F3, F4, F5, F6) Second focused review of the previous fix-up commit surfaced seven findings. Fixing five; F2 (atomic ApplySettings) is pre-existing and masked by clone-before-apply at every call site, F7 (recover boundary in WithSharedParser) is hypothetical with no current trigger. F1 + F4: lint.UnmarshalFrontMatter conflated "no front matter" with "front matter that decoded into a zero struct". A misspelled key (dropping a letter from "diagnostics", a schema-mismatched field) or an empty `---\n---\n` block left fm.Settings == nil && fm.Diagnostics == nil, so the integration fixture loader silently accepted malformed bad fixtures. - UnmarshalFrontMatter now returns (body, hadFrontMatter, err). Callers that want to enforce schema check hadFrontMatter rather than inspect v's zero state. - integration/rules_test.go's parseFixtureFrontMatter uses the new bool. A bad fixture with malformed FM now fails loudly with the correct "missing front matter" message. - Unit tests in lint/frontmatter_test.go pin all four cases (valid block, no block, empty fences, unrecognised keys, decode error). F3: markdown.Splice's docstring promised "ascending and non-overlapping" but the implementation enforced neither — a violating edit list crashed inside body[prev:e.Start] with an opaque "slice bounds out of range" panic. Added an entry-point precondition check that panics with a descriptive message naming the offending edit's index, Start, and End. Three new sub-tests in TestSpliceInvariantViolation pin the message text so any future change to the check surfaces here. F5: taskCheckBoxEdits's comment claimed "a malformed AST cannot panic the fix". Technically true after the nil/empty-Lines guards landed in commit 08446eb, but the guards do NOT close the silent-corruption case where NearestBlockAncestor skips an empty-Lines TextBlock and returns the enclosing ListItem — start+3 then deletes the bullet instead of the checkbox. Documented the goldmark TextBlock invariant the fix relies on and the failure mode when a hand-built AST violates it. F6: fixGitHubAlerts's local (Paragraph, non-empty Lines) re-check was dead code today and would silently skip a fix if flavor.IsGitHubAlert ever drifted — the worst failure mode for a fix path (diagnostic flagged but fix did nothing). Removed the local check; the comment documents why trusting IsGitHubAlert is the right call and which test pins the contract. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Close the F5 silent-corruption path in taskCheckBoxEdits The previous round's nil/empty-Lines guards did NOT prevent the silent-corruption case the F5 finding pointed at: NearestBlockAncestor SKIPS ancestors with empty Lines() and keeps walking up, so a TaskCheckBox under a Paragraph-with-empty-Lines under a ListItem-with-populated-Lines yields block=ListItem and start = bullet-position, not '['-position. The guards both pass; start+3 deletes three bytes from the wrong block. Add an explicit `f.Source[start] != '['` check that declines the edit when the byte at Lines.At(0).Start is not the bracket the task-list parser's invariant promises, plus a bounds check on start+3 against len(f.Source) for the truly-short-source case. Three new red/green tests pin the guard: - non-bracket start (paragraph with arbitrary Lines) - nil block ancestor (orphan TaskCheckBox) - bracket runs past EOF (2-byte source) https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Address round-3 review: Splice negative-Start, IsGitHubAlert contract pin Recall agent surfaced three findings after commit 17cca70; one was the F5 silent-corruption path already closed in commit 131a9f1. The remaining two land here. #2: markdown.Splice's precondition check fired for negative-Start edits via the generic "overlaps previous edit ending at 0" panic. A producer that subtracts past 0 and emits Start=-1 sent the debugger chasing a non-existent previous edit. Add a dedicated `Start < 0` guard that names the actual fault, plus a pin in TestSpliceInvariantViolation. #3: fixGitHubAlerts removed the local (Paragraph, non-empty Lines) re-check and trusts flavor.IsGitHubAlert's contract — but no behavior test pinned that contract. The four existing fix tests exercise inputs where IsGitHubAlert returns true via the real parser; they would not catch a future relaxation of IsGitHubAlert that returns true for a non-Paragraph first child. Add TestIsGitHubAlertContractPostcondition: walk a corpus of alert / non-alert / degenerate blockquotes, and on every IsGitHubAlert==true case assert FirstChild is *ast.Paragraph with non-empty Lines. Coverage in pkg/markdown and pkg/markdown/flavor stays at 100%; mdsmith check and golangci-lint clean. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti * Mark plan/185 done Every task and acceptance criterion is checked off and the implementation has passed three rounds of recall review with all follow-up fixes landed. Flip the status from 🔳 to ✅ and refresh the PLAN.md catalog. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti --------- Co-authored-by: Claude <noreply@anthropic.com>
* Render summary front-matter as inline Markdown in lead and index
single.html and list.html emitted the summary via {{ . }}, so a
front-matter value like "Use \`<?catalog?>\` ..." rendered with
literal backticks instead of <code> tags. feature-grid.html already
ran the same field through .RenderString — only the docs and list
templates lagged.
Switch both to RenderString with display=inline (so no nested <p>
forms inside the wrapping element). Add a verify-website-links probe
asserting at least one rendered <p class="lead"> contains a <code>
tag; goodSite carries the fixture and a new failure test pins
regression detection.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Add template-source check for summary front-matter rendering
The verify-website-links probe catches the bug at rendered-HTML
stage, but only after a Hugo build. This Go test walks
website/layouts/*.html directly and fails the moment a template
references .Params.summary without going through .RenderString
(or being a plain `if .Params.summary` predicate). baseof.html is
exempt — its meta-description fallback intentionally renders
plain text because meta tags don't accept HTML.
Verified the check fires on the exact bug pattern by temporarily
restoring `{{ with .Params.summary }}<p>{{ . }}</p>{{ end }}` in
single.html: the test failed with the file:line of the violating
expression.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Add lead-with-code fixture to verify-website-links happy-path test
cmd/mdsmith-release/main_test.go has its own minimal Hugo-output
fixture for TestRunVerifyWebsiteLinksHappyPath, parallel to
goodSite() in internal/release/verifylinks_test.go. The new
"summary front-matter renders inline markdown as <code>" probe
requires at least one rendered <p class="lead"> with a <code>
tag, so the e2e test tree needs the same fixture goodSite already
carries.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Wrap directory-creation slice to satisfy lll (120 col)
Adding the lead-fixture directory pushed the inline literal past
the 120-column limit golangci-lint enforces. Pull the slice into a
named local.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Address copilot review: loosen lead-probe regex; whole-file template scan
Two threads on PR #408:
1. verifylinks.go probe regex was too tight: required <p class=lead>
to have no extra attrs/classes and <code> to be the first nested
element. Broadened to allow extra <p> attrs, additional classes
alongside `lead` (quoted or unquoted), and arbitrary inline tags
before <code>, while still anchoring to the same <p> block via
`(?:[^<]|<[^/]|</[^p]|</p[^>])*` so code in a sibling <p> does
not satisfy the probe. Added two tests:
- AcceptsLooseHTMLShapes covers extra classes, unquoted class,
attrs-before-class, and a leading <a> tag before <code>.
- RejectsCodeOutsideLead pins the anchor: code in a sibling <p>
still fails.
2. template_summary_test was per-line, so a multi-line `{{ with\n
.Params.summary }}` would slip through. Switched to whole-file
scan (FindAllStringIndex on the full content, line number from
byte offset). Added DetectsMultiLineWith to pin the new behavior.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* docs: add summary front-matter rendering section to website-config
Document the two safety checks added in this PR — the template-source
Go test and the rendered-HTML probe — under a new section in
docs/development/website-config.md. Names what data must satisfy
what condition (the three allowed action shapes, the exemption for
baseof.html, the anchor of <code> to the lead <p>).
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Drop rendered-HTML lead probe; rely on template-source test
Reviewer flagged the verify-website-links lead probe as
content-dependent (PR #408 review): if every docs summary
ever drops its code spans, the probe fails even though the
templates are correct. The template-source Go test in
internal/release/template_summary_test.go already enforces
the .RenderString invariant at authoring time and does not
depend on what authors write in summaries, so it's the
single source of truth.
Removes:
- The "summary front-matter renders inline markdown as
<code>" probe and its 2-paragraph header comment.
- The lead-with-code fixture from goodSite() and the
AcceptsUnquotedHref test in internal/release.
- The lead fixture from TestRunVerifyWebsiteLinksHappyPath
in cmd/mdsmith-release (back to 4-dir inline literal).
- FailsOnLiteralBackticksInLead, LeadProbe_AcceptsLooseHTMLShapes,
and LeadProbe_RejectsCodeOutsideLead — all probe-specific tests.
- The "Rendered-HTML probe" section in docs/development/
website-config.md.
Also rewords the template_summary_test.go header and assert
message to drop the ephemeral branch identifier (flagged by
the same review). The comment now describes the regression
self-contained, not by branch name.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Tighten template-source check: summary must be a RenderString argument
Reviewer flagged that the previous regex (`\.RenderString\b`)
treated any action containing both `.RenderString` and
`.Params.summary` as safe — even if the two only co-occurred
(e.g. `{{ if eq .Params.summary .RenderString }}` would pass).
Replace with a regex that requires a method-call relationship:
either RenderString first and summary after (positional argument),
or summary first piped (one or more `|`) into RenderString. The
positional form matches every current template
(`.RenderString (dict ...) .Params.summary`); the piped form
covers `.Params.summary | .RenderString`-style alternatives.
Added TestSummaryFrontMatterCheck_RequiresRenderStringArgument
with 8 sub-cases pinning both safe forms (positional, piped) and
the rejected patterns (bare output, with rebind, co-occurrence
in a comparison without pipe).
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Rewrite summary-rendering check with AST-aware tokenizer
Addresses 15 issues from the multi-angle code review. The previous
regex scanner had real correctness gaps and ergonomic limitations.
Bugs the old scanner had:
1. Brace inside string literals (e.g. `{{ printf "{%s}" .Params.summary }}`)
silently skipped the action — the naive `{{[^{}]*}}` regex stopped
at the inner `{`.
2. Positional renderSummary regex matched when `.Params.summary` was
nested in a non-RenderString call: `{{ .RenderString (printf "%s"
.Params.summary) }}` falsely passed.
3. Piped renderSummary regex matched any later `.RenderString`
mention: `{{ .Params.summary | print "x" .Page.RenderString }}`
falsely passed because `.RenderString` appears after the pipe.
4. ifPredicate anchored to exact `if .Params.summary`, so compound
forms (`if and .Params.summary $cond`) and `else if` were flagged
as violations even though they only check presence.
5. Variable assignment `{{ $s := .Params.summary }}` was flagged
indiscriminately; subfield access `{{ if .Params.summary.X }}`
was flagged because the predicate regex required exact end.
6. Hugo comments mentioning the field (`{{/* .Params.summary */}}`)
were treated as live references.
The new scanner:
- Tokenizes actions with quote-aware lexing (handles double-quoted
and backtick strings; `{` and `}` inside strings no longer split
actions) — fixes #1.
- Strips `{{/* ... */}}` comments before scanning (preserves line
numbers via newline padding) — fixes #6.
- Classifies each action by leading keyword. `if` / `else if` /
`range` are presence predicates regardless of compound form or
subfield access — fixes #4 and the subfield case. `with` /
`else with` are flagged as rebinding the dot.
- Pipeline analysis: splits on `|` at paren-depth 0, walks each
stage. `.Params.summary` is safe only when it is a top-level
positional argument to `.RenderString` or the head of a pipeline
whose terminal stage is `.RenderString`. Nested references inside
parens are flagged — fixes #2 and #3.
Other fixes in this commit:
- `baseof.html` exemption is now by relative path
(`_default/baseof.html`), not basename. Hugo idiomatically
supports per-type baseof overrides; basename-only would silently
exempt all of them.
- The walker collects I/O errors into a side slice and continues,
so a transient ReadFile failure on one file no longer masks
every violation in the remaining files.
- `layoutsPath` deleted; the walk calls the existing `repoRoot(t)`
helper from messaging_test.go.
- Three duplicated regex blocks collapsed into package-level vars
and a single `scanSummaryViolations` helper. The multi-line test
no longer round-trips through a tempfile.
- baseof.html's meta description pipes the summary through
`$.RenderString (dict "display" "inline") . | plainify` so
backticks become `<code>` HTML and then plain text. SEO
snippets ship clean prose instead of literal Markdown
punctuation.
- The pipe-form alternation (YAGNI in the previous regex) is
generalized by the pipeline walker; no current template uses
the piped form but the scanner handles it correctly when one
does.
Verified by: 19-case `TestClassifyAction_TableDriven` covering
every safe and unsafe shape, plus three scenario tests
(`TestFindActions_BalancedStrings`,
`TestScanSummaryViolations_CommentsIgnored`,
`TestScanSummaryViolations_MultiLineWith`,
`TestScanSummaryViolations_BraceInString`). The real-layout walk
(`TestSummaryFrontMatterRenderedThroughRenderString`) passes
against the current `website/layouts/` tree, and the rendered
meta description on the progressive-disclosure page now shows
"Use <?catalog?> ..." rather than "Use \`<?catalog?>\` ...".
docs/development/website-config.md updated to enumerate the safe
and forbidden forms and explain the baseof.html meta-description
plainify path.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Replace hand-rolled tokenizer with text/template/parse-based AST walker
The previous regex/tokenizer scanner had real correctness gaps surfaced
by the second code review (15 findings). Most traced back to one root
cause: parsing Hugo templates with hand-rolled string scanning is
fundamentally brittle. text/template/parse with parse.SkipFuncCheck
parses every layout in website/layouts/ cleanly (verified across all
24 .html files), so this commit replaces ~250 lines of tokenizer with
a ~150-line AST walker. The hand-rolled findActions, scanOneAction,
skipDoubleQuoted, skipBacktickQuoted, splitPipeStages, firstToken,
summaryHits, computeDepths, summaryRefRe, summaryCommentRe, and
summaryAssignRe are all gone.
Bugs the AST walker fixes:
1. Case sensitivity (e.g. `.params.summary`): now matched via
strings.EqualFold on FieldNode.Ident.
2. Brace-in-string actions (`{{ printf "{%s}" .Params.summary }}`):
the parser handles string literals natively; the action is an
ActionNode whose pipe references .Params.summary.
3. RenderString nested in another call: the AST walker traces sub-
PipeNode args, so `{{ .RenderString (printf "wrapper: %s"
.Params.summary) }}` is correctly safe (summary feeds the
sub-pipe whose output goes to RenderString) and
`{{ printf "%v %v" (.RenderString "foo") .Params.summary }}` is
correctly forbidden (.RenderString is a value, not a call).
4. Piped to wrong function: pipeOutputsSummaryViaRenderString
walks stages and tracks whether summary's value flows into a
.RenderString call, returning false for
`{{ .Params.summary | print "x" .Page.RenderString }}` where
.RenderString is just a value passed to print.
5. Qualified receivers: cmdIsRenderString now recognises FieldNode
(.RenderString, .Page.RenderString), ChainNode (chained access),
and VariableNode ($.RenderString — text/template parses the
dollar-context form as a VariableNode with Ident=["$",
"RenderString"], a detail confirmed by AST dump).
6. Subfield access in pipelines: fieldIsSummary matches any
FieldNode whose Ident starts with [Params, summary], so
`{{ .Params.summary.HTML | .RenderString }}` is safe.
7. CRLF inside multi-line actions: the lexer handles whitespace;
no hand-rolled boundary set to maintain.
8. Comments mentioning the field: ParseComments is not set, so
comments are dropped at parse time and never visited.
9. Variable assignment bypass: pipeAssignsSummary inspects
PipeNode.Decl across IfNode/WithNode/RangeNode/ActionNode
contexts, so `{{ if $s := .Params.summary }}` is flagged.
10. Range over string: RangeNode whose Pipe references
.Params.summary is forbidden (range rebinds . to each rune).
11. Post-render filters: pipelineOutputsSummaryViaRenderString
walks stages forward and allows any sequence after
.RenderString, so `{{ ... | .RenderString | plainify }}` and
`{{ $.RenderString (dict) .Params.summary | plainify }}` are
safe — exactly the form baseof.html now uses.
12. baseof.html exemption removed. The meta-description fallback
used `{{ with .Params.summary }}{{ . }}` which the scanner
cannot follow (the dot is opaque). Switched to an `if`/
`else if` chain where each branch references its source
explicitly. Now baseof.html is scanned natively — no
path-based exception. All four sources (.Description,
.Params.summary, .Params.description,
.Site.Params.description) flow through the same
$.RenderString | plainify projection, so any Markdown in
any source ships as clean plain text.
13. classifyAction-called-twice eliminated (the walker visits
each node once and accumulates violations).
14. computeDepths over-allocation eliminated (no depth array;
the AST distinguishes nested calls structurally).
15. Test surface compressed: the 19-row TestClassifyAction table
is replaced by a 28-row TestScanSummaryViolations table that
runs full templates end-to-end through the same code path the
main test uses, plus three scenario tests (multi-line with,
CRLF, unterminated parse error). The findActions/balanced-
strings/comments tests vanish — those properties belong to
the parser now.
Verified: all 28 table cases pass, hugo --minify rebuilds the site
cleanly, the rendered meta description on the progressive-disclosure
page reads "Use <?catalog?> ..." (plain text, no backticks),
mdsmith-release verify-website-links exits 0, `mdsmith check .`
passes on all 373 files.
The docs/development/website-config.md page is updated to enumerate
the new safe and forbidden forms and explain the four-branch
meta-description projection.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Surface filepath.Rel errors in the layouts walk instead of silently dropping them
Reviewer flagged on PR #408 that `rel, _ := filepath.Rel(...)` swallowed
the error path. If the relative-path computation ever fails (e.g. an
unexpected mount, symlink target outside layoutsDir), the violation
output would lose file context and the test would still pass.
Handle the error the same way the surrounding walker handles other I/O:
record it in ioErrors and skip the file. The post-walk assertion fails
the test if anything landed in ioErrors, so the diagnostic is always
actionable.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Walk every parse tree, not just the root; detect qualified field access
CRITICAL FIX: scanSummaryViolations was only walking tree.Root, which
for Hugo layouts that use `{{ define "main" }}...{{ end }}` blocks
(every page-rendering layout: index.html, _default/list.html,
_default/single.html, rule/single.html) contains only the whitespace
between defines — none of the actual rendering logic. The body of each
define lives in a separate tree in the treeSet. The walker never
visited it, so the test was silently passing without scanning any of
the layouts that matter.
Verified by injecting `{{ with .Params.summary }}<p class="lead">...`
into list.html line 25 (a direct rebind, the exact regression the test
is supposed to catch). Before fix: test passed despite the violation.
After fix: test correctly fails with `_default/list.html:25: with
.Params.summary rebinds the dot...`.
The fix iterates every tree in treeSet so define-block bodies are
walked. The wrapping tree (treeSet[path]) is also visited but contains
only text fragments outside the defines, so no duplicate violations.
Added regression test TestScanSummaryViolations_DefineBlock to pin the
behavior.
Two other gaps closed while here:
1. VariableNode handling. `$.Params.summary` (the dollar-context
value reference) parses as a VariableNode with Ident `["$",
"Params", "summary"]` — argReferencesSummary did not recognise
VariableNode and so silently skipped any value-reference form
(e.g. `{{ $.Params.summary }}` would have shipped raw output
without being flagged).
2. Qualified field access. `.Page.Params.summary` parses as a
FieldNode with Ident `["Page", "Params", "summary"]`. The
previous fieldIsSummary required Ident[0] == "Params" exactly
and so missed Page-qualified accesses. Generalised to
identsReferenceSummary, which finds the `Params` → `summary`
adjacency anywhere in the chain.
Added TestScanSummaryViolations_QualifiedFieldAccess with four sub-
cases covering both forms safe and unsafe.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Handle TemplateNode: `{{ template "x" .Params.summary }}` is a rebinding
The previous code-review surfaced that `{{ template "name" pipe }}`
and its `{{ block "name" pipe }}` shorthand pass the pipe value
as the sub-template's dot. The sub-template's body sees `.` as the
bound value — the same rebinding `with` does, but across a tree
boundary. The walker did not visit TemplateNode at all, so an
invocation like `{{ template "summary-box" .Params.summary }}` was
silently safe.
baseof.html uses `{{ block "main" . }}{{ end }}` (passing the page,
not summary) so today no template is affected, but the gap is real.
Added the case and a regression test covering three shapes (summary
in template pipe → flagged, unrelated value → safe, block with
summary → flagged).
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Add scanned-file counter and pin multi-line violation line number
Two follow-ups from the code review:
1. Test could pass vacuously if `website/layouts/` ever disappears
or the walker is misconfigured. Counter checks at least 5 .html
files were scanned (the four page-rendering layouts plus
baseof.html).
2. text/template/parse sets WithNode.Pos to the start of the pipe
(the `.Params.summary` operand), not the `{{` opener. For the
multi-line fixture `<p>\n{{ with\n .Params.summary }}\n...`,
the reported line is 3 (the operand), not 2 (the `{{`). This
is more diagnostic — readers see exactly which value is the
problem. Pin the assertion at line 3 with a comment explaining
the choice.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Sort treeSet keys; recurse into ChainNode receiver and sub-pipe Decls
Three follow-ups from the sweep round of the code review:
1. Map iteration over treeSet was non-deterministic. A template
with multiple `{{ define ... }}` blocks whose violations land
in different defines produces a violations slice in shuffled
order across runs, hurting triage. Sort the keys before walking.
2. argReferencesSummary's ChainNode case checked the trailing
Field chain via chainIsSummary but never descended into the
parenthesised receiver. `(.Params.summary).Foo` parses to
ChainNode{Field:["Foo"], Node:PipeNode{...summary...}} and was
silently safe. Recurse into n.Node.
3. pipeAssignsSummary only inspected the outer pipe's Decl, so
`{{ .RenderString (dict) ($s := .Params.summary) }}` slid past
the var-assignment guard — the bound `$s` is a name that
escapes the per-action scan, the very pattern the rule forbids.
Walk sub-pipelines too.
Added two regression tests: TestScanSummaryViolations_ChainReceiver
and TestScanSummaryViolations_SubPipeVarAssign.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Move scanner from internal/release/_test.go to internal/templatecheck
Scanner code was hiding in a _test.go file in the release package
— a frontend-template contract owned by release tooling, where
authors editing website/layouts/ wouldn't think to look. Move it
to a new package and reduce the release-side file to its
integration role.
New package internal/templatecheck:
- Exports Scan(path, content) ([]Violation, error)
- Exports Violation
- All helpers (walker, walk, check*, *ReferencesSummary,
cmdIsRenderString, identsReferenceSummary, pipeAssignsSummary,
pipeOutputsSummaryViaRenderString) are package-private
- Unit tests (table-driven + scenarios) live alongside the code
internal/release/template_summary_test.go shrinks to one
function: TestSummaryFrontMatterRenderedThroughRenderString,
which walks website/layouts/ and calls templatecheck.Scan on each
file. The release package still owns "did the website pass the
contract"; the contract definition lives where contributors can
find it.
docs/development/website-config.md updated to point at both files
— the classifier package and the integration test — and the
"extend the AST classifier" pointer now refers to the new
package.
No behavior change. All 40+ unit tests in templatecheck pass; the
release integration test still walks every .html file under
website/layouts/ and the scanned-file counter remains.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Cover the templatecheck defensive branches to fix the codecov/patch gate
The new package shipped at 90.4% coverage because nil-pipe and
empty-Ident guards in the unexported helpers had no test driving
them — naturally unreachable through real Hugo AST input but
required to lift the patch number to project baseline (99.54%).
Added TestHelpers_DefensiveBranches with 12 white-box subtests
that synthesise the edge cases directly: nil PipeNode passed to
each predicate, empty CommandNode.Args, FieldNode/ChainNode/
VariableNode with zero-length Ident/Field, unknown node type
into argReferencesSummary, lineOf with a position past content
end, walk(nil) at both interface and typed-nil levels. Also
covered three real-but-untouched paths: ChainNode whose trailing
field chain itself carries `Params.summary`, the recurse into
ChainNode.Node receiver for `(.Params.summary).Foo`, and
`.X | someFunc .Params.summary | .RenderString` where the value
arrives via a middle stage's Args[1:].
Result: package now at 100.0% line coverage.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
* Address review angles A, D, Sweep: chain boundary, var-assign through render, docs
Multiple findings from the angles:
A1 — ChainNode boundary case (`(.Params).summary`): the
`Params`/`summary` adjacency straddles the receiver/Field
boundary. Neither half alone has the pair. Added tailIdents
to flatten a chain receiver into one Ident slice and check
for the pair across the boundary. Caught: bare ChainNode,
nested ChainNode `((.A).Params).summary`, and function-call
receiver via fallback recurse.
A2 — `{{ $s := .RenderString .Params.summary }}` was being
flagged as variable assignment of the raw summary, but the
RHS routes summary through RenderString so the bound name
holds template.HTML (rendered Markdown). Hugo emits
template.HTML without re-escaping, so a later `{{ $s }}`
ships rendered output. pipeAssignsSummary now skips the
flag when the RHS already outputs via RenderString.
Sweep #1 — Forbidden-forms list in docs was missing the
TemplateNode case (`{{ template "name" .Params.summary }}`
and the `{{ block }}` shorthand). Added.
Sweep #2 — Vacuous-pass guard was `scanned >= 5` but the
tree has 24 .html files. Raised to 20.
Sweep #3 — `assert.Empty(t, formatted)` had no diagnostic
message; sibling `ioErrors` line did. Added.
Sweep #4 / D1 — Commented the deliberate case-sensitivity
asymmetry: identsReferenceSummary uses EqualFold (Hugo
Params map is case-insensitive); cmdIsRenderString uses
`==` (Go method names are case-sensitive). Without the
comment a maintainer "normalising" one would silently
break the rule.
Sweep #7 / D6 — Commented the inner `if n == nil` guard
inside the *parse.ListNode case. The typed-nil ElseList
of an if-without-else bypasses the outer guard; removing
the inner check would re-introduce a panic.
tailIdents — pruned the unreachable VariableNode case (no
template syntax produces a ChainNode whose receiver is a
bare VariableNode; `$s.Field` parses as VariableNode with
the field appended to its own Ident). Per CLAUDE.md, no
defensive branch without a red/green driver.
Coverage: templatecheck package now at 100.0% of statements.
https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
---------
Co-authored-by: Claude <noreply@anthropic.com>
Plan 209 task 5, mirroring the kind-file integration pair. - contract: layout/basename, bad basename, subdirectory, dual-source, built-in-name collision, unknown key, .yaml/.yml collision, both extensions, SourcePath populated, and SourcePath surviving Load -> Merge -> ResolveFile (the CLI path). - equivalence: a file convention and the equivalent inline convention emit byte-equal diagnostics on the same doc (acceptance criterion #1), with a non-empty guard against a trivial match. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462
) * Start plan 209: Convention-per-file config under `.mdsmith/conventions/` * config: add discoverConventions for .mdsmith/conventions/ Mirror discoverKinds (plan 208) for user-defined conventions: walk .mdsmith/conventions/*.{yaml,yml} at the workspace root, decode each into a UserConvention with strict KnownFields decoding plus the anchor/alias guard, and key the result by basename. Reject bad basenames, subdirectories, .yaml/.yml collisions, and unknown top-level keys, each error naming the offending file. Add SourcePath to UserConvention (parallel to KindBody.SourcePath) so a later commit can surface a convention's defining file in provenance output. Plan 209 task 1. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * config: merge .mdsmith/conventions/ files into Load Wire discoverConventions into Load: tag inline conventions with the loaded config path for uniform provenance, then merge file-defined conventions from .mdsmith/conventions/<name>.yaml. A name colliding with a built-in convention, or with an inline convention, is a config error that names the offending file(s) so the user can resolve it. Plan 209 task 2. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * config: fold inline kind/convention tagging into merge helpers Loads funlen budget (60 lines) was exceeded once plan 209s convention block landed in Load. Move the inline-entry SourcePath tagging out of Load and into mergeKindFiles and mergeConventionFiles, ahead of each helpers empty-discovery early return, so Load just delegates to the two helpers. Behavior is unchanged: inline kinds and conventions are still tagged with the config path before file-defined entries merge, so a name-collision diagnostic still quotes both sources. No helper has a direct unit test; all SourcePath assertions go through Load. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * test(config): tighten plan-209 convention-file coverage; fix stale comment From a two-pass code review of the convention-files diff: - builtin-collision test iterates convention.Names() instead of a hardcoded {portable,github,plain} subset, so newer built-ins (obsidian, parity) are covered and a reserved-set drift is caught. - DiscoverConventions_LoadsFullBody now asserts body.SourcePath (the field that actually flows into cfg.Conventions), not just the redundant discoveredConvention.sourcePath. - add TestLoad_InlineAndFileConventionsCoexist: a non-colliding inline convention and a .yml file convention both survive the merge with correct SourcePaths, guarding the nil-map merge and the .yml Load path. - fix a stale doc comment in parseKindFile (UnmarshalSafe -> RejectYAMLAliases) to match the actual call and the convention sibling. No production behavior change. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * config: harden convention-file reading (.mdsmith/conventions/) Five robustness fixes from the code review (the kind-file sibling gets the same in the next commit): - size cap: parseConventionFile reads via readLimitedConfig (1 MB cap) instead of an unbounded os.ReadFile, matching how .mdsmith.yml is read. - reject symlinks: a symlink reports IsDir()==false (lstat), so it bypassed the subdirectory guard and a symlinked file could be read off the workspace. Now rejected with a clear error. - accept .yaml/.yml case-insensitively so a .YAML file is not silently skipped. - deterministic collision errors: iterate discovered conventions in sorted name order so the reported file is stable across runs rather than depending on map order. - empty/comments-only file now errors with a clear "empty convention file" message instead of the decoder bare "EOF". (An empty file cannot be a no-op convention: applyConvention requires a flavor, so a clear error is the right outcome.) https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * config: harden kind-file reading (.mdsmith/kinds/) Apply the same five robustness fixes as the convention sibling to the kind-file path, keeping the two in parity: - size cap via readLimitedConfig (1 MB) instead of an unbounded os.ReadFile. - reject symlinks under .mdsmith/kinds/ (a symlink bypassed the subdirectory guard via lstat IsDir()==false). - accept .yaml/.yml case-insensitively so a .YAML file is not silently skipped. - deterministic collision errors: iterate discovered kinds in sorted name order. - empty/comments-only file errors clearly ("empty kind file") instead of the decoder bare "EOF" (empty kind files already errored; this only clarifies the message). Tests added for each; full suite green. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * config,kindsout: report active convention + its source path in kinds resolve Plan 209 tasks 3-4 (provenance + CLI). `mdsmith kinds resolve <file>` now prints the active convention and, for a user convention, the file that defined it — parallel to how each kind shows `defined-in`. - config: ResolveFile populates a new FileResolution.Convention (ResolvedConvention: name, user flag, source path) via resolveConvention. A built-in is reported by name with no path; no selection yields the zero value. - kindsout: WriteFileResolutionText prints a `convention:` line; --json carries a `convention` object with name + user + source-path (mirroring each resolved kind's source-path). - merge: copyUserConventions dropped UserConvention.SourcePath, so the path was blanked once the CLI merged loaded config onto defaults. Copy it through, matching copyKinds. Tests: resolveConvention unit cases; kindsout text + JSON (user, built-in, none); copyUserConventions preserves SourcePath. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * test(integration): convention-file contract + equivalence tests Plan 209 task 5, mirroring the kind-file integration pair. - contract: layout/basename, bad basename, subdirectory, dual-source, built-in-name collision, unknown key, .yaml/.yml collision, both extensions, SourcePath populated, and SourcePath surviving Load -> Merge -> ResolveFile (the CLI path). - equivalence: a file convention and the equivalent inline convention emit byte-equal diagnostics on the same doc (acceptance criterion #1), with a non-empty guard against a trivial match. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * docs: convention files under .mdsmith/conventions/ Plan 209 task 6. New reference page modeled on kind-files.md: directory layout, file shape (flavor + rules), basename/extension/subdirectory/symlink rules, composition with .mdsmith.yml (inline coexistence, dual-source + built-in-name errors), a "split an inline convention" recipe, and an Audit section showing the kinds-resolve convention line + --json convention object. Also: a .mdsmith/conventions/ row in the cross-system boundaries table, a cross-link from the conventions reference, and the regenerated catalog/include blocks (index.md, CLAUDE.md, AGENTS.md, copilot-instructions.md). https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 * plan 209: complete — convention-per-file config All six in-scope tasks done and all acceptance criteria verified (byte-equal effective rules, dual-source/basename/subdir/built-in collision errors, kinds-resolve source-path, full test suite, golangci-lint, mdsmith check). Status 🔳 → ✅; regenerated the PLAN.md index. https://claude.ai/code/session_01NGDnJbnrZFW2SsP66QY462 --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
This PR extends the line-length rule (TM001) with three new configuration options that provide more granular control over line length checking:
heading-maxandcode-block-maxallow different maximum lengths for heading and code block linessternoption that only flags lines exceeding the limit if they contain a space character past the limit columnKey Changes
New Rule fields:
HeadingMax *int: Optional maximum length for heading linesCodeBlockMax *int: Optional maximum length for code block linesStern bool: Enable strict space-based flaggingEnhanced line classification:
collectHeadingLines()to identify heading lines via AST walkinglineCategoriesstruct for cleaner organizationbuildCategories()to pre-compute line classificationsImproved checking logic:
activeMax()determines the effective limit for each line based on its typeisSkipped()consolidates all skip conditions including new stern mode logichasSpacePastLimit()implements stern mode detectionSettings handling:
ApplySettings()into smaller focused methods (applyMax(),applyPositiveIntPtr(),applyStern(), etc.)heading-maxandcode-block-maxmust be positive integersDefaultSettings()to includestern: falseDocumentation and tests:
Implementation Details
maxwhen unset, maintaining backward compatibilityexcludecompose correctly: excluded lines are still skipped even when a category-specific limit is sethttps://claude.ai/code/session_01PNAbRKSSUqz9rtkM37eKiM