Add TM021 table-format rule for consistent markdown table formatting - #3
Merged
Conversation
jeduden
force-pushed
the
claude/plan-28-keNKp
branch
from
February 13, 2026 12:40
89865ce to
82afafe
Compare
Add a new lint rule that checks and auto-fixes markdown table formatting to produce aligned, readable tables with consistent column widths and padding. Tables inside fenced code blocks are skipped. Implements: rule.Rule, rule.FixableRule, rule.Configurable Category: table Setting: pad (int, default 1) — spaces on each side of cell content https://claude.ai/code/session_01HkWm2GJinJRX5zJK8ziJQK
TM019 (catalog) now generates tables pre-formatted with TM021 (table-format) settings, preventing the "generated section is out of date" diagnostic when both rules are active. Added exported FormatString and GetPad to the tableformat package for cross-rule use. Updated all markdown files to comply with TM021 formatting. https://claude.ai/code/session_01HkWm2GJinJRX5zJK8ziJQK
…include) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeduden
force-pushed
the
claude/plan-28-keNKp
branch
from
February 13, 2026 13:05
82afafe to
8f3bcb9
Compare
jeduden
pushed a commit
that referenced
this pull request
Apr 11, 2026
- 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
jeduden
pushed a commit
that referenced
this pull request
Apr 17, 2026
- 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
jeduden
pushed a commit
that referenced
this pull request
May 7, 2026
…errors Three remaining unreachable lines after the FS+Runner refactor: - buildOneWheel's outDir/.staging-<plat> mkdir failure — fixed the existing TestBuildWheelsFailsOnStagingMkdir which counted off by one (staging is MkdirAll call #4, not #3). - copyDir's recursive call error — stage src with a subdir so the inner copyDir is invoked, then fail the inner copyFile to bubble the error back through the outer call. - moveWheels' listWheels error — direct fault test that asserts a ReadDir failure surfaces as a moveWheels error rather than a silent no-op. Coverage on internal/release: 98.2% → 99.1%. Remaining lines are physically unreachable: main()'s 1-line os.Exit wrapper, run()'s os.Getwd error path (cwd was deleted under us), and copyDir's e.Info() race (entry vanished between ReadDir and Info). https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6
jeduden
added a commit
that referenced
this pull request
May 8, 2026
…239) * Plan 130: ship npm + PyPI distribution and a tag-driven version stamp Adds the npm root + platform-package layout, the PyPI wheel skeleton, the VS Code Marketplace + Open VSX publish steps, and a smoke-test job that asserts mdsmith version matches the tag on every channel. scripts/set-version.sh rewrites every tracked manifest from the 0.0.0-dev sentinel to the cleaned tag; scripts/check-versions.sh runs in a new ci.yml version-guard so a hand edit on main fails fast. asdf and mise registry submissions are still pending and remain follow-ups in plan/130 because they live outside this repo. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Address Copilot review on PR #239 - set-version.sh: accept SemVer with both pre-release and build metadata (1.2.3-rc.1+build.5) and fail fast when a required manifest is missing instead of silently skipping. - check-versions.sh: detect a deleted optionalDependencies key in npm/mdsmith/package.json so the guard catches drift, not just pin mismatches. - build-wheels.sh: retag wheels with `python -m wheel tags` so the dist-info/WHEEL metadata matches the filename instead of leaving it stamped py3-none-any. Install `wheel` alongside `build` in the pypi job. - pyproject.toml: drop the comment that mentioned a non-existent marker file. - release.yml smoke-test: switch the npm channel from node:lts-alpine to node:lts so the default bash shell exists before the install step runs. - internal/release: add TestBuildWheelsLayout that exercises build-wheels.sh and asserts the WHEEL metadata reflects each platform tag. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Address Copilot review pass 2 on PR #239 - check-versions.sh: fail fast when a tracked manifest is missing, matching set-version.sh's require_file behavior. A renamed or deleted manifest now blows up the version-guard CI job rather than silently passing. - release.yml smoke-test: wrap the npm and mise install commands in the same retry-with-backoff loop the pip channel uses, since npm registry propagation can lag the publish by ~60s. - internal/release: add TestCheckVersionsFailsOnMissingManifest. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Address Copilot review pass 3 on PR #239 - README.md: drop the misleading mise/asdf one-liner. Until the registry/plugin follow-ups land, the short forms don't resolve. - docs/guides/install.md: replace the mise row with the working `mise use -g ubi:jeduden/mdsmith@latest` form (no registry PR required), drop the asdf row from the quick-start table, and flag both the asdf section and the short mise form as pending-follow-up. - release.yml smoke-test: switch the mise channel from `mdsmith@VER` to `ubi:jeduden/mdsmith@VER` so the smoke test passes on the very first tag, before the mise registry PR. - build-wheels.sh: wrap each per-wheel build in a subshell with an EXIT trap so a `python -m build` failure under `set -e` still cleans up the staging directory. The previous RETURN trap only fired on a normal return. - ci.yml test job: install build/wheel/hatchling so the new TestBuildWheelsLayout actually runs in CI instead of skipping. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Fix bogus action SHA pins so the test job can resolve setup-python The setup-python pin I added in the previous commit pointed at a SHA that does not exist on actions/setup-python, causing the `test` CI job to fail with "Unable to resolve action" before any step ran. Same problem for the setup-node and gh-action-pypi-publish pins introduced in the npm/pypi jobs. Re-pin each to a real SHA looked up via the GitHub API, with versions matching what the rest of the repo uses for adjacent actions: - actions/setup-python@a309ff8b … # v6.2.0 (matches setup-go v6.2.0) - actions/setup-node@48b55a01 … # v6.4.0 (latest) - pypa/gh-action-pypi-publish@6733eb7d … # v1.14.0 (latest) https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Address Copilot review pass 4 on PR #239 - set-version.sh: pre-flight grep before each rewrite. If a manifest is missing the expected version field (or the npm root has no @mdsmith/* optionalDependencies block), abort with an actionable error instead of letting perl no-op silently and shipping 0.0.0-dev. Same guard is added to rewrite_pyproject_version. - release.yml smoke-test: track an `ok` flag inside each retry loop and `exit 1` after the loop if no attempt succeeded. The previous `if cmd; then break; fi` pattern fell through under bash -e, so a registry that stayed unresolvable would surface later as an opaque "mdsmith: command not found" instead of "install never succeeded". - internal/release: add two regression tests covering the new rewrite-fails-fast paths. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Address Copilot review pass 5 on PR #239 - set-version.sh: tighten the SemVer regex to reject leading zeros in MAJOR/MINOR/PATCH (per semver.org grammar). Both npm and PyPI reject 01.2.3-style tags downstream; refuse them here so the failure has an actionable message. - check-versions.sh: name the drifted pin in error output, e.g. "@mdsmith/linux-x64 pin '1.2.3'" instead of just "pin '1.2.3'", so a multi-pin drift produces actionable lines. - internal/release: add TestSetVersionRejectsLeadingZero and update TestCheckVersionsRejectsOptionalDepDrift to assert on the new error format. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Address Copilot review pass 6 on PR #239 - set-version.sh: tighten the SemVer regex to the full semver.org grammar. Pre-release identifiers that are purely numeric must not have leading zeros (so "-01" is rejected, but "-rc01" stays valid because it is alphanumeric). Build metadata identifiers keep the spec-mandated leading-zero allowance. - docs/guides/install.md: clarify the PyPI console script runs the binary via os.execv on POSIX and subprocess.run on Windows, matching the actual entrypoint instead of implying execv on every platform. - internal/release: add TestSetVersionAcceptsValidSemverShapes and extend TestSetVersionRejectsLeadingZero to cover prerelease numeric identifiers. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Rename npm root to @mdsmith/cli (unscoped 'mdsmith' is taken) The unscoped `mdsmith` package on npm was registered by another project (afroldo, 2026-01-31, "CLI para gerar READMEs e arquivos Markdown"), so we cannot publish a root named `mdsmith`. The `@mdsmith` scope we already use for the platform sub-packages is free, so the root publishes there as `@mdsmith/cli`. The installed binary is still called `mdsmith` because the package's `bin` field maps the command name independent of the package name. Users run `npm install -g @mdsmith/cli` and the `mdsmith` CLI lands on `$PATH` exactly as before. Updated: - npm/mdsmith/package.json `name` and the README's install snippet - README.md install quick-start - docs/guides/install.md table row, npm section, and prose - release.yml smoke-test (npm channel pulls @mdsmith/cli@VER) - plan/130 npm strategy section + acceptance criteria The bun shim tests didn't need changes — they only exercise the platform-package resolver, which already lives under @mdsmith/. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Guard Marketplace/Open VSX publish secrets before continue-on-error The two extension-publish steps run with `continue-on-error: true` so a transient Marketplace or Open VSX outage does not block the downstream `release` job. The same flag would also hide an unset or empty `VSCE_PAT` / `OVSX_PAT`, leaving us with a "successful" release that never reached either marketplace. Add a `Verify Marketplace and Open VSX tokens are set` step (no continue-on-error) that fails the job if either secret is empty. The publish steps themselves stay tolerant of registry errors, so misconfiguration fails fast while outages still allow the GitHub release to ship. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Replace bash release scripts with cmd/mdsmith-release Go CLI The four bash scripts (set-version, check-versions, build-npm-platforms, build-wheels) used perl regex to edit JSON and TOML manifests, which was both fragile and indirectly tested — Go tests shelled out to bash and grepped the output, so a regression pointed at the wrapper rather than the bug. Port the logic to internal/release/ (version.go, buildnpm.go, buildwheels.go) and expose it through a separate cmd/mdsmith-release binary with four subcommands (stamp, check, build-npm, build-wheels). The release workflow invokes the Go CLI via `go run ./cmd/mdsmith-release ...`. Tests now exercise the exported functions directly. Behaviour parity: - Stamp's SemVer regex stays at the full semver.org grammar (rejects leading zeros in MAJOR/MINOR/PATCH and in numeric prerelease identifiers). - Pre-flight checks still fail fast when a manifest is missing, has no version field, or — for the npm root — drops its @mdsmith/* optionalDependencies block. - Check accumulates every problem instead of exiting on the first, so a multi-pin drift produces multiple actionable lines. - BuildWheels still orchestrates `python -m build` and `python -m wheel tags` (no native Go wheel builder exists), but the staging directories now clean up on failure via Go `defer` instead of bash's leak-prone `trap RETURN`. - Test for BuildWheels skips when python/build/wheel/hatchling aren't on PATH; the test job in ci.yml already installs them. Net: -3 toolchain (bash + perl + grep) and +1 (Go), with the JSON/TOML editing now happening in code that compiles, lints, and tests under the same toolchain as the rest of the repo. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Disable setup-go cache in release.yml + raise release-toolkit coverage zizmor flagged the three new actions/setup-go invocations I added to the vscode, npm, and pypi jobs as cache-poisoning surfaces. The existing build job already uses cache: false; mirror that. Codecov also dropped on the patch — exercise the easy uncovered paths in cmd/mdsmith-release and internal/release: - cmd/mdsmith-release: cover bad-arity dispatcher branches for every subcommand, the invalid-version stamp path, and the reportError nil/non-nil exit-code mapping. - internal/release: cover BuildNpmPlatforms when the npm root manifest is missing, BuildWheels when the python source is missing, the LICENSE-copy branch, and ValidateSemver on empty input. Coverage on the new code: internal/release 79.4% → 81.8%, cmd/mdsmith-release 50.0% → 81.6%. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * mdsmith-release: switch to pflag and add helper-level coverage The CLI now uses spf13/pflag with one FlagSet per subcommand, matching the conventions in cmd/mdsmith/main.go (subcommand Usage, ContinueOnError, the reportFlagParseErr helper that pretty-prints non-help parse errors). None of the subcommands take flags today; the FlagSets exist so adding one later is a local edit and so help/error messages match the user-facing binary. Also raise patch coverage on the release toolkit by exercising the small helpers directly: - listWheels (empty, mixed-extension, missing dir) - moveWheels (empty staging, multi-file relocate) - copyDir (nested tree) - stagePythonTree (missing asset) - BuildWheels (missing artifact, no python invocation needed) - subcommand --help and --bogus dispatch (covers reportFlagParseErr) Coverage on the new code: internal/release 81.8% → 84.6%, cmd/mdsmith-release 81.6% → 88.9%. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * testify on release tests + spin asdf/mise out into plan/145 - Convert internal/release and cmd/mdsmith-release tests to use testify per CLAUDE.md ("require for preconditions that abort, assert for checks that continue"). Behaviour unchanged; just shorter, more idiomatic test code. - Spin the asdf-plugin repo (jeduden/asdf-mdsmith) and the mise-plugins/registry submission out of plan/130 into plan/145 — they ship in separate repos and don't gate the multi-channel release this branch enables. Plan/130 now points at plan/145 for the asdf+mise narrative; plan/145 owns the task list and acceptance criteria for both registry submissions plus the doc/smoke-test cleanup that follows. - mdsmith fix . to keep PLAN.md and the catalog in CLAUDE.md/docs/guides/index.md in sync. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Push CLI dispatcher coverage to 94% + drop bogus author from pyproject - cmd/mdsmith-release tests now exercise the success branch of runCheck (println "all manifests pinned at"), the happy-path build-npm dispatch end-to-end, and reportError-translated failures for both build-npm and build-wheels. Coverage on the CLI: 88.9% → 94.4%. - python/pyproject.toml: remove the made-up `authors = [...]` entry I added when scaffolding the wheel. The PyPI metadata field is optional; until the real author info is provided, no entry is better than a wrong one. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Cover release-toolkit error branches: 95.8% CLI, 88.3% library CI's codecov gate failed because patch coverage (79.82%) was well below the project's auto-target (95.14%). Push the new code's coverage by exercising the small but numerous IO error returns directly: - internal/release helpers: stage / list / move / copy / retag primitives all get direct error-path tests (missing source, rename target as a file, missing dir, etc.). - BuildNpmPlatforms: cover the per-platform mkdir-fails branch by colliding the platform path with a regular file. - TrackedManifests: pin the readdir-of-platform-subpackages branch so the slice growth path is observable. - cmd/mdsmith-release: direct unit test for the reportFlagParseErr nil branch and end-to-end dispatch paths that surface reportError-translated failures for build-npm and build-wheels. Coverage: cmd/mdsmith-release 88.9% → 95.8%, internal/release 84.6% → 88.3%. Remaining uncovered statements in the library are IO-error returns (mkdir failure mid-stage, write failure) that need OS-level fault injection to trigger reliably; those will catch up when codecov re-measures with the new tests. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * release: route all IO through an injectable FS interface The codecov gate kept failing because the release-toolkit's IO error returns (mkdir-failure mid-pipeline, ReadFile/WriteFile faults, Rename onto a non-directory) cannot be triggered against a healthy filesystem, leaving entire `if err != nil { return err }` arms uncovered. Introduce a small writable FS interface alongside the existing read-only fs.FS pattern in the rest of the codebase. The Toolkit struct holds an FS field, defaults to osFS in production, and accepts a fault-injecting fakeFS in tests so every error-return branch is exercised. - internal/release/fs.go — FS interface + osFS impl + Toolkit - internal/release/{version,buildnpm,buildwheels}.go — methods on Toolkit; package-level Stamp / Check / BuildNpmPlatforms / BuildWheels keep the original signatures by delegating to a default Toolkit. - internal/release/fault_test.go — fakeFS with per-method failOn{N}Call counters plus a battery of fault-injection tests covering each IO error return. Coverage on the package: 88.3% → 94.2%. The remaining holes are the python-orchestration branches (require python on PATH) and two unreachable defaults. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * release: route python invocations through an injectable Runner The remaining patch-coverage gap was the python-orchestration failure branches in buildwheels.go (runPythonBuild's "python -m build" failed, retagWheels' "python -m wheel tags" failed). Both paths require python on PATH AND a way to make python fail — hard to drive against a real interpreter. Add a tiny Runner interface alongside FS: type Runner interface { RunCommand(dir, name string, args ...string) error } Toolkit grows a runner field; New() defaults to osRunner, tests construct via NewWithDeps(fakeFS, fakeRunner). runPythonBuild becomes a method on Toolkit; retagWheels switches its inner exec.Command to t.runner.RunCommand. Three new fault tests (runPythonBuild fail, retagWheels fail, buildOneWheel propagates python-build fail) close the orchestration-failure coverage holes. Coverage on internal/release: 94.2% → 95.6%. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * release: cover the manifest-write and listWheels-skip-dir cases Two small gaps left after the FS / Runner refactor: - buildOneNpmPlatform 93.8% — the package.json WriteFile failure (the second per-platform write, between the binary copy and an optional LICENSE) had no fault test. Add one. - listWheels 90% — the IsDir-skip branch (a directory whose name ends in `.whl`) was never exercised. Drop a `subdir.whl/` into the filter test so the skip path runs. Coverage on internal/release: 95.6% → 96.4%. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * release: cover the retagWheels and moveWheels error branches in buildOneWheel After the FS+Runner refactor, the only buildOneWheel statements left uncovered were the two error returns following runPythonBuild: retagWheels-fails and moveWheels-fails. Both need a wheel actually present in staging so the helpers have work to do (otherwise their inner loops short-circuit and never trigger the failure paths). Add a preStageWheel helper that drops a real fake.whl into the staging dir before the test calls buildOneWheel, then arm either the Runner (for retagWheels) or FS.Rename (for moveWheels) to fail. Coverage on internal/release: 96.4% → 96.9%. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * release: cover the no-version-field and copyDir-mkdir branches Two unreached statements left after the buildOneWheel coverage push: - checkManifest's "no version field found" note() — a manifest that exists but has no top-level version key. Add a Check test against an edited fixture. - copyDir's MkdirAll(dst) failure — an FS where ReadDir(src) succeeds but the destination mkdir fails. Use the existing failOnMkdirAllCall harness. Coverage on internal/release: 96.9% → 98.2%. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * release: cover staging-mkdir, recursive-copyDir, and moveWheels-list errors Three remaining unreachable lines after the FS+Runner refactor: - buildOneWheel's outDir/.staging-<plat> mkdir failure — fixed the existing TestBuildWheelsFailsOnStagingMkdir which counted off by one (staging is MkdirAll call #4, not #3). - copyDir's recursive call error — stage src with a subdir so the inner copyDir is invoked, then fail the inner copyFile to bubble the error back through the outer call. - moveWheels' listWheels error — direct fault test that asserts a ReadDir failure surfaces as a moveWheels error rather than a silent no-op. Coverage on internal/release: 98.2% → 99.1%. Remaining lines are physically unreachable: main()'s 1-line os.Exit wrapper, run()'s os.Getwd error path (cwd was deleted under us), and copyDir's e.Info() race (entry vanished between ReadDir and Info). https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * docs/install: point asdf+mise follow-up callouts at plan/145 The asdf-plugin repo and mise-registry submission moved out of plan/130 into plan/145 in an earlier commit, but the install guide's two "Pending follow-up" callouts still linked back to plan/130. Update both references so readers land on the right tracking plan. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Address Copilot pass: 100% release coverage + 5 doc/code nits Remaining reachable lines in internal/release got direct fault tests (buildOneWheel propagates stagePythonTree failure, copyDir propagates DirEntry.Info() error via a bespoke errInfoEntry). internal/release coverage: 99.1% → 100.0%. Then five Copilot review comments on top: - internal/release/version.go: clarify the TrackedManifests comment about npm/platforms/. BuildNpmPlatforms writes to its outDir argument (npm/dist in CI), not to npm/platforms/; the helper just stamps platform sub-package manifests if some prior step happened to materialise them. - internal/release/buildwheels_test.go: fix the assert.Failf mis-call. failureMessage is the title; %q verbs belong in the msg+args parameters, not the title. - internal/release/buildwheels.go: pick python executable at runtime via pythonExecutable() (python first, fall back to python3) so the binary works on hosts that only ship python3. Test gating uses the same resolver. - docs/guides/install.md: the direct-download snippet now curl's checksums.txt before sha256sum -c so a copy-paste install actually works. - PR description (via gh pr edit): drop the scripts/{set,check}-version.sh references, mention the Go cmd/mdsmith-release CLI, and link plan/145. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 * Fix BuildWheels source-tree path regression + plan/130 stale ref Two Copilot findings: - internal/release/buildwheels.go: the previous over-eager `replace_all` of "python" → pythonExecutable() also rewrote `filepath.Join(rootDir, "python")`, which is the source-tree path. On hosts where `python` isn't on PATH (Debian/Ubuntu with only python3) BuildWheels would have looked for <rootDir>/python3 instead of <rootDir>/python and failed every wheel build. Revert that one site to the literal "python". - TestBuildWheelsFailsWhenPythonSourceMissing now also asserts the error names <root>/python so the same regression cannot return. - plan/130: the vscode-publish task still said run `set-version.sh` before vsce package; flip the wording to `mdsmith-release stamp` to match the implemented workflow. https://claude.ai/code/session_015MPUo4nJ4iySQES6J3ByQ6 --------- Co-authored-by: Claude <noreply@anthropic.com>
jeduden
pushed a commit
that referenced
this pull request
May 27, 2026
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
jeduden
pushed a commit
that referenced
this pull request
May 27, 2026
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
jeduden
pushed a commit
that referenced
this pull request
May 27, 2026
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
jeduden
pushed a commit
that referenced
this pull request
May 28, 2026
… 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
jeduden
pushed a commit
that referenced
this pull request
May 28, 2026
… 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
jeduden
pushed a commit
that referenced
this pull request
May 28, 2026
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
jeduden
pushed a commit
that referenced
this pull request
May 28, 2026
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
jeduden
pushed a commit
that referenced
this pull request
May 28, 2026
… 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
jeduden
added a commit
that referenced
this pull request
May 28, 2026
…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>
jeduden
added a commit
that referenced
this pull request
May 28, 2026
* 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>
5 tasks
jeduden
pushed a commit
that referenced
this pull request
Jul 10, 2026
Four findings from an 8-angle xhigh review of the pass-1 follow-up
commit, each verified before fixing:
1. internal/mdpath.IsMarkdownPath could misdetect an extension on a
Windows-native path (produced by filepath.FromSlash) when an earlier
directory segment also contained a dot, because path.Ext never
treats '\' as a separator. Rewrote to find the last '.' after the
last occurrence of either '/' or '\', independent of host OS —
correct for both fs.WalkDir's always-forward-slash paths and
crossfilereferenceintegrity's OS-native link paths. Added regression
cases covering the divergence.
2. requiredstructure/alloc_test.go's TestCollectBodySyncPoints_NoByte
SplitAlloc had no raceEnabled skip guard, unlike every other
alloc-gate test this PR touches (pre-existing on main, not
introduced by this PR, but touched by the pass-1 commit's added
`nil` argument — fixed while already editing this line).
3. buildFieldPattern's redundant `if cache != nil` guard on the read
path was unnecessary (a nil Go map read is always safe) — and the
eager `make(map[string]*regexp.Regexp)` at the parseSchemaWithRootFS
call site paid an allocation even for schemas with zero {field}
text, the common case.
4. The bare `map[string]*regexp.Regexp` cache parameter, threaded
through 4 function signatures with the nil-means-uncached contract
repeated in each doc comment, is now a small fieldPatternCache type
with get/put methods: the zero value is nil-receiver-safe (get/put
both handle a nil *fieldPatternCache), and the backing map is
allocated lazily on first put instead of eagerly — resolving both
the redundant-check and eager-allocation findings from #3 in one
change, and centralizing the contract in one place instead of 4.
All fixes verified: go build, go vet, go test ./... (including
-race), golangci-lint (0 issues), mdsmith check . (544 files, 0
failures).
jeduden
pushed a commit
that referenced
this pull request
Jul 12, 2026
Four findings from an 8-angle xhigh review of the pass-1 follow-up
commit, each verified before fixing:
1. internal/mdpath.IsMarkdownPath could misdetect an extension on a
Windows-native path (produced by filepath.FromSlash) when an earlier
directory segment also contained a dot, because path.Ext never
treats '\' as a separator. Rewrote to find the last '.' after the
last occurrence of either '/' or '\', independent of host OS —
correct for both fs.WalkDir's always-forward-slash paths and
crossfilereferenceintegrity's OS-native link paths. Added regression
cases covering the divergence.
2. requiredstructure/alloc_test.go's TestCollectBodySyncPoints_NoByte
SplitAlloc had no raceEnabled skip guard, unlike every other
alloc-gate test this PR touches (pre-existing on main, not
introduced by this PR, but touched by the pass-1 commit's added
`nil` argument — fixed while already editing this line).
3. buildFieldPattern's redundant `if cache != nil` guard on the read
path was unnecessary (a nil Go map read is always safe) — and the
eager `make(map[string]*regexp.Regexp)` at the parseSchemaWithRootFS
call site paid an allocation even for schemas with zero {field}
text, the common case.
4. The bare `map[string]*regexp.Regexp` cache parameter, threaded
through 4 function signatures with the nil-means-uncached contract
repeated in each doc comment, is now a small fieldPatternCache type
with get/put methods: the zero value is nil-receiver-safe (get/put
both handle a nil *fieldPatternCache), and the backing map is
allocated lazily on first put instead of eagerly — resolving both
the redundant-check and eager-allocation findings from #3 in one
change, and centralizing the contract in one place instead of 4.
All fixes verified: go build, go vet, go test ./... (including
-race), golangci-lint (0 issues), mdsmith check . (544 files, 0
failures).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a new linting rule (TM021) that enforces consistent column widths and padding in markdown tables, following prettier-style formatting conventions.
Key Changes
New rule implementation (
internal/rules/tableformat/rule.go)::---,:---:,---:)Check()for diagnostics andFix()for automatic formattingComprehensive test suite (
internal/rules/tableformat/rule_test.go):Documentation (
rules/TM021-table-format/README.md):Test fixtures:
Integration:
cmd/tidymark/main.gointernal/config/load.gointernal/config/config_test.goandinternal/integration/rules_test.goNotable Implementation Details
>) and list indentation prefixeshttps://claude.ai/code/session_01HkWm2GJinJRX5zJK8ziJQK