Skip to content

Commit a1aa6c5

Browse files
jedudenclaude
andauthored
Vendor Punkt sentence tokenizer fork into internal/punkt (#367)
* Plan 193 task 1: BenchmarkRule_MDS024 with shared abbr-heavy corpus Adds a per-Check allocation gate for MDS024 and a shared test corpus helper used by both BenchmarkSplitSentences_Subset and the new BenchmarkRule_MDS024. The smoke ceiling is 1500 allocs/op (above the ~1118 baseline) so a regression on the unmigrated rule trips; task 12 will flip the constant to 10 once the internal/punkt rework cuts the per-call cost. * Plan 193: rework MDS024 to fit ≤ 10 allocs/op budget Fork a minimal trained-Punkt segmenter into internal/punkt/ that is byte-identical to upstream over the equivalence corpus but allocation-clean per call. The per-token machinery that drove the old ~1118 allocs/op count is gone — Token carries no regex pointers, Type runs a byte scanner into a pooled buffer, the TokenGrouper reuses its pair slice across the three Annotate passes, the collocation key is assembled in a reusable buffer and the lookup relies on `m[string(b)]` map elision, and the per-call working state lives in a sync.Pool. mdtext.SplitSentences now dispatches via splitSentences, which the default build implements through internal/punkt and the upstream tag (mdtext_punkt_upstream) implements through neurosnap/sentences for A/B comparison. Measured: 7 allocs/op for MDS024.Check on a warm File, verified by both BenchmarkRule_MDS024 (b.Fatalf gate) and TestCheckAllocBudget. BenchmarkSplitSentences drops from 593 to 22 allocs/op; the abbr-heavy subset drops from 1082 to 16. Engine corpus benchmarks stay well within budget. * Plan 193: replace stale results with fresh apples-to-apples numbers The previous Results table mixed plan 191's old upstream-CPU numbers (2.10 GHz Xeon, different day) with new default-build numbers from a different 2.80 GHz CPU, producing a spurious "corpus got slower" line. Re-measured both build tags on the same 2.10 GHz Xeon in the same minute and tabulated the median p95 ranges. Segmenter benchmarks confirm the rework: BenchmarkSplitSentences −63% time / −96% allocs, BenchmarkSplitSentences_Subset −71% time / −98.5% allocs. Corpus benchmarks are flat as expected — MDS024 is opt-in, so the corpus runs do not exercise the segmenter at all. * Address Copilot review on PR #367 Six review threads + codecov gate, all addressed: - token.go: hasSentEndChars uses strings.Contains instead of []byte(tok)/[]byte(p) per iteration; both conversions were copying allocations on a hot path. - annotate.go: state.reset() now clears the used ranges of s.tokens, s.ptrs, s.pairs, and s.sents so Token.Tok and Sentence.Text references do not survive across sync.Pool reuse. New TestState_ResetClearsTokAndTextReferences pins the contract. - storage.go: CollocationIndex / HasCollocation were built-and-unused dead code — the runtime path goes through Collocations[string(buf)] map-elision. Dropped them; doc.go and tests updated to describe the actual lookup path. - sentence_equivalence_test.go: BenchmarkSplitSentences_Subset comment no longer references the deleted fastMultiPunctWordAnnotation; now points at internal/punkt's multiPunctAnnotation and MatchAbbrPattern. - doc.go: copies the upstream MIT license into internal/punkt/UPSTREAM_LICENSE (no .md extension so mdsmith's content rules do not lint verbatim license text). Pointer in doc.go updated. - word_tokenizer.go: TestTokenize_TrailingMultiByteRuneMatches\ Upstream pins the matched word-tokenizer quirk between the fork and upstream for trailing multi-byte runes; TestTokenize_TrailingMultiByteSentenceIncludesTail documents that the downstream sentence-emitter fallback covers the dropped tail so user-visible output is unaffected. The reviewer's correctness concern is preserved-upstream behavior, recorded as such. Coverage: tokenizer.go's NewEnglish panic branch was unreachable from tests; extracted loadEnglishStorage so the panic can be driven red/green with malformed bytes. Three dead-code branches removed per CLAUDE.md's defensive-code rule (an unreachable len==0 check, an always-false numEnd guard, and an always-false core>=trim guard in numericTail). The remaining branches got dedicated unit tests in annotate_test.go (group, isAllASCIILower, every reachable branch of tokenAnnotation and multiPunctAnnotation). Patch coverage on internal/punkt/ is now 100%. * Append vendored neurosnap/sentences MIT notice to root LICENSE MIT requires the upstream copyright + permission notice in copies of the software, so distributing mdsmith with internal/punkt/ vendored in needs the upstream notice surfaced at the project root, not only alongside the vendored code. Adds a THIRD-PARTY LICENSES section under the root mdsmith MIT, pointing at the matching internal/punkt/UPSTREAM_LICENSE file for code-locality reference. * Address Copilot review + ship vendored MIT notice via wheel and npm root Copilot threads: - TokenizeInto's fallback was keyed off `len(dst) == 0`, which only fires for callers passing an empty dst. An external caller passing a non-empty dst plus whitespace-only text would lose the upstream fallback append. Switched to `len(dst) == orig` with `orig` taken at entry. New test pins the contract. - testcorpus's empty-corpus test was mutating the exported AbbrHeavy variable, which races other packages' parallel `go test ./...` consumers under `-race`. Lifted the join logic into joinWithSpace (unexported), tests now exercise it directly without touching the global. License attribution (blind-reviewed): - Python wheel: stagePythonTree now copies the root LICENSE into the staged tree so hatchling's `license-files = ["LICENSE"]` picks it up and writes it into each wheel's .dist-info/. Without this, the PyPI tarball would ship without mdsmith's MIT notice OR the embedded third-party attribution for internal/punkt/. - npm root: the @mdsmith/cli root package directory has no checked-in LICENSE. Added a workflow step that copies the root LICENSE into npm/mdsmith/ before `npm publish`, so the published tarball carries the vendored MIT notice. The platform sub-packages already do this via buildOneNpmPlatform. - pyproject.toml: declares `license-files = ["LICENSE"]` per PEP 639. Test coverage: - TestStagePythonTree_CopiesLicense / TestStagePythonTree_MissingLicenseIsOK pin the wheel-LICENSE staging contract. - The pre-existing fault-injection test updated its ReadFile call index from 2 to 3 to account for the new LICENSE-read step (which is best-effort and swallows errors, matching buildnpm's pattern). * Address Copilot doc-nit review Four review threads, all comment/doc only: - internal/testcorpus/abbr.go: joinWithSpace's precomputed capacity now matches the joined length exactly (sum(len(s)) + len(items)-1 for separators), so the comment's "sized exactly" claim is true. - internal/punkt/doc.go: the hyphenation-scan bullet referenced bytes.IndexByte, but the implementation uses strings.LastIndexByte (avoids the []byte conversion). Updated. - internal/punkt/annotate.go: the typeAnnotation comment carried the same stale bytes.IndexByte reference; updated. - internal/punkt/annotate.go: tokenAnnotation's leading comment still mentioned a Storage.CollocationIndex map that no longer exists (dropped in an earlier round of this review). Rewritten to describe the actual Storage.Collocations + map-elision path. No behavioural change; tests / lint / mdsmith check all green. * Address Copilot review (round 4) Three legitimate findings: - internal/testcorpus/abbr.go: joinWithSpace now uses strings.Builder with a pre-sized Grow so the "single backing array" comment is true. The previous `string(out)` from a byte slice was an extra copy; Builder.String() returns the backing array directly without copy. - internal/punkt/token.go: isListNumber's `.?` step advanced by one byte, but Go regex `.` matches a rune, not a byte. A multi-byte token like "1世" would match the upstream regex but not the byte scanner. Switched to utf8.DecodeRuneInString for the optional-any step and added "1世", "12世", "1世)", "1世)abc" to the scanner corpus so the regex-equivalence oracle and the fuzzy enumeration pin the matched semantics. - internal/release/buildwheels.go: stagePythonTree was swallowing every ReadFile error on the root LICENSE. The "swallow on NotExist" intent was right, but the implementation hid real failures (perm denied, transient I/O) — which would silently ship a wheel without the required MIT notice. The branch is now a typed switch: NotExist tolerated, every other error propagates wrapped with the path. The WriteFile branch is also wrapped. Two new fault tests pin both branches. No behavioural drift on the happy path; allocs/op gate on MDS024 unchanged at 7. * Address Copilot review (round 5) Four findings: - internal/release/buildwheels.go: stagePythonTree now fails loudly on any LICENSE read error, including NotExist. pyproject.toml's `license-files = ["LICENSE"]` makes the file required — hatchling fails the build downstream anyway — and the symmetric upfront check gives a clearer error than the hatchling complaint. Prevents mis-staging from silently producing a wheel without the required MIT notice. - internal/release/fault_test.go: TestStagePythonTreeFailsOnLicense\ ReadIOError had a stray os.WriteFile to a different TempDir and a comment about NotExist "hiding the injection" that misdescribed fakeFS (which injects before delegating). Removed dead write, tightened comment. New stagePythonSrc helper centralises the <root>/python + <root>/LICENSE setup so the many fault tests that exercise stagePythonTree don't each rewrite the fixture. - internal/punkt/annotate.go: multiPunctAnnotation's leading comment referenced the deleted mdtext fastpunct.go. Rewritten to point at the current home of the DFA (internal/punkt/abbr.go / plan 191). - internal/punkt/annotate.go: the dead-IsInitial-guard breadcrumb also pointed at fastpunct.go. Replaced with the analysis inline so the reasoning stays with the elision and doesn't depend on an external file. Test fixture changes: - fixtureManifests now writes a LICENSE so wheel-side fault tests routed through it (TestBuildWheelsFailsOnStagingMkdir, …) don't trip the new hard-failure branch. - The direct-stagePythonTree fault tests use stagePythonSrc instead of the previous inline temp-dir setup. - TestBuildWheelsLayout writes a LICENSE so its end-to-end run with real python clears the new check. No behavioural change on the happy path; allocs/op gate on MDS024 unchanged at 7. * Address Copilot review: drop redundant hasSuffix helper The hasSuffix helper in internal/punkt/token.go was line-for-line identical to strings.HasSuffix, and its leading comment justified the wrapper by claiming callers should not "pull strings into this file". That rationale stopped being true earlier in this PR when hasSentEndChars switched to strings.Contains — both token.go and annotate.go now import "strings" anyway. Removed the helper and called strings.HasSuffix directly at the four call sites (hasUnreliableEndChars, hasSentEndChars, typeAnnotation, multiPunctAnnotation). * Address Copilot review: make AbbrHeavy corpus immutable by construction AbbrHeavy was an exported mutable []string. Any caller could accidentally assign through it (testcorpus.AbbrHeavy[0] = "") and silently skew benchmarks for every other parallel consumer. The previous round already moved the empty-corpus test off the global, but the variable remained a shared mutable surface. - Renamed the package-level corpus to unexported abbrHeavy. - AbbrHeavy() is now a function that returns slices.Clone(abbrHeavy) — every caller gets an independent slice, so mutation cannot leak. - AbbrHeavyParagraph reads abbrHeavy directly (it joins and returns a string; no exposed slice). - TestAbbrHeavy_ReturnsFreshCopy pins the immutability contract. - mdtext's BenchmarkSplitSentences_Subset clones once into a local and iterates the local, so the clone cost is not charged per benchmark iteration (it would be a constant per-N anyway, but pulling it outside b.N + adding b.ResetTimer keeps the measured cost focused on SplitSentences itself). MDS024 alloc gate unchanged at 7/10; no behavioural change to production code (testcorpus is consumed only by tests/benchmarks). * Restore CJK terminal-punctuation support in the segmenter fork internal/punkt's word tokenizer was English-only — every '.'/'!'/'?' was a sentence boundary but the CJK full-width variants `。!?` were treated as regular content. A user enabling MDS024 on Chinese or Japanese Markdown would have a whole paragraph collapse into one "sentence" (the segmenter never finds a boundary), hiding the "too many sentences" diagnostic and exaggerating the "sentence too long" diagnostic. The fix mirrors upstream's IsCjkPunct path: - word_tokenizer.go: IsCjkPunct is restored. TokenizeInto splits on unicode.IsSpace OR IsCjkPunct, and when the boundary is a CJK rune the cursor advances past it so the punctuation stays with its preceding token (the same `i += utf8.RuneLen(char)` trick upstream uses). punctSentenceEnders includes `。!?` so hasSentencePunct flags CJK-terminated words for the getNextWord gate. - token.go: HasPeriodFinal accepts `。` as a period suffix (3-byte UTF-8 sequence), matching upstream. - paragraphstructure/rule.go: cheapBounds was counting only ASCII enders. A long Chinese/Japanese paragraph would clear the guard (sentUB=1) and the segmenter would never run — false negative on the rule. Added the CJK trio to the count. Test-pyramid additions: - TestIsCjkPunct pins the four-rune set against drift. - TestTokenize_CjkPunctuationSplitsTokens cross-checks the word-token-level output against upstream on five CJK samples. - TestTokenize_CjkParagraphsMatchUpstream pins the Sentence-level output (what MDS024 actually consumes) on five CJK paragraphs including a mixed CJK+ASCII case. - TestHasPeriodFinal table now includes CJK cases. - TestHasSentencePunct table now asserts the CJK enders match. - TestCheapBounds table now exercises CJK-only and mixed CJK+ASCII paragraphs. - mdtext's sentence_equivalence_test corpus picked up three CJK paragraphs so the integration gate fires under both build tags. Fixtures (folder format — replaces single good.md / bad.md): - good/{english,chinese,japanese}.md — three short, well-structured paragraphs the rule must NOT flag. - bad/{english,chinese,japanese}.md — three 7-sentence paragraphs that must trip the "paragraph has too many sentences (7 > 6)" diagnostic. (For English the legacy fixture had 8 sentences and is preserved; the new CJK fixtures use 7.) MDS024 alloc gate unchanged at 7/10. * Address Copilot review (round 7) + measure realistic per-file cost Reviewer pointed out that the previous warm-File measurement excluded the per-File memo build cost MDS024 pays when it is the first or only paragraph-aware rule on a file. That's the production reality, and the warm-File gate was hiding it. Switched to cold-File measurement (fresh lint.File per iteration, parse-only baseline subtracted) so the gate reflects the rule's actual per-file cost. With a representative single abbreviation-heavy paragraph (~100 bytes, 2–3 sentences — the first entry of testcorpus.AbbrHeavy rather than the artificial 8-paragraph join), MDS024 cold = 10 allocs/op, exactly at the CLAUDE.md ceiling. Two small allocation cuts landed to make 10 reachable: - mdtext: replaced sync.Once.Do(initTokenizer) with sync.OnceFunc(initTokenizer). The bare Do allocates one func-value boxing per call (Go escape analysis can't elide the parameter), which the alloc-budget gate picks up. OnceFunc constructs the wrapper at package init. - punkt.Tokenizer.Tokenize: replaced `defer func() { ... }()` with explicit cleanup at the bottom of the function. The deferred closure was escaping to the heap (1 alloc/op) and there is no panic-recovery path that needs defer. - paragraphstructure: replaced two fmt.Sprintf calls in the diagnostic-message construction with string concatenation via strconv.Itoa. Sprintf cost ~3 allocs per call; concat with cached small-int Itoa lowers to a single allocation. The plan's allocation-budget table is updated to honestly describe the cold-memo breakdown. * Drop cold-File alloc budget to 9 (strictly below CLAUDE.md ceiling) User feedback: the cold budget should sit strictly below the CLAUDE.md ≤ 10 ceiling, not at it. The previous round landed at exactly 10 allocs/op (= ceiling, no headroom for regressions). Three further cuts get us to 9: - lint.File.Memo: dropped sync.Once.Do, which allocated a func value per call (the `func() { e.val = build() }` closure captures `e` and `build`, both escape-tracking pointers, so Go's escape analysis can't elide it). Replaced with an atomic.Bool + mutex double-checked-lock pattern: cheap atomic load on the warm path, mutex-guarded build on the cold. - lint.File.MemoFile (new): the *File-passing variant of Memo so astutil.CollectSectionParagraphs can register a package-level builder (buildSectionParagraphs) instead of a closure that captures `f`. The closure was costing 1 alloc per Check. - mdtext.SplitSentencesInto (new): pool-friendly variant that appends to a caller-provided []string. paragraphstructure now borrows a []string from a sync.Pool, fills it, uses it, then Puts it back before checkParagraph returns. The bare `make([]string, 0, len(sents))` SplitSentences would do is amortized across calls. Cold-File measurement on a representative paragraph drops from 10 to 9 allocs/op. The budget constant is tightened to 9 so any regression that re-allocates anywhere along this path fails the gate before the rule crosses the CLAUDE.md ceiling. All existing tests pass under both build tags. The plan's budget table is updated to describe the new breakdown and the cuts. * Address Copilot review (round 9) Four findings: - rule.go:121: the strconv.Itoa "returns from cache" claim was imprecise — Go's strconv builds new strings for each call. Reworded the comment to focus on Sprintf's real cost (the format-string scratch buffer + per-arg boxing) versus string-concat's single concatstrings allocation. The alloc-saving conclusion stands; just the rationale was framed wrong. - rule.go:156: sentBufPool.Put kept the previous paragraph's string headers reachable via the pooled slice. sync.Pool could pin large input buffers across Check calls. Now clear(sentences) runs before the truncate-and-Put so the pool only holds zero-valued slots. - internal/rules/MDS024-paragraph-structure/README.md and internal/punkt/doc.go: the docs claimed full-width `!` and `?` were sentence boundaries. Empirically (cross-checked against upstream English pipeline), only `。` flags a sentence break — `!`/`?` are word boundaries through IsCjkPunct but never reach HasSentEndChars, since the English WordTokenizer's enders set is ASCII-only. Both docs rewritten to say so, plus practical guidance for CJK Markdown authors: use `。` between sentences for the rule to count them. No behaviour change; alloc-budget gate still passes at 9 allocs/op. * Address Copilot review (round 10) with red/green TDD Three findings, each driven through a failing test first: - internal/punkt/annotate.go typeAnnotation: dropped the trailing-period rune by BYTE INDEX, which mangles a CJK 。 (3-byte) into an invalid UTF-8 prefix and would skip a CJK-prefixed abbrev lookup. Added TestTypeAnnotation_CjkPeriodStrippingDropsFullRune which seeds AbbrevTypes with "中文" and expects "中文。" to match (Abbr=true). The test FAILS red against the byte-strip (verified locally by temporarily reverting the fix) and PASSES green against the new utf8.DecodeLastRuneInString path. The ASCII case is pinned by TestTypeAnnotation_AsciiPeriodStrippingUnchanged so the rune-aware strip can't regress the common path. - internal/rules/paragraphstructure cheapBounds: the terminal-punct rune set counted full-width ! and ? as sentence boundaries, but the English Punkt pipeline (the only one mdtext.SplitSentences runs) does NOT mark them as SentBreak — only 。 does, via HasPeriodFinal. The rune set is now `. ! ? 。` only, matching actual segmenter behaviour. New test TestCheapBounds_FullWidthExclamQuestionNotSentBreaks asks mdtext.SplitSentences directly: text containing only !/? between clauses must segment as ONE sentence; pins the invariant cheapBounds relies on. The TestCheapBounds table also picked up "问题?回答。继续!" expecting sentUB=2 (the one 。), not 4 (which the previous "counts everything" behaviour would have produced). - internal/mdtext/fastpunct_init.go header comment said initialization happens "under initOnce", but mdtext.go switched to sync.OnceFunc (`initTokenizerOnce`). Comment updated to name the current mechanism. No behavioural change on the alloc-budget gate: still 9 allocs/op cold-File. * Add coverage for SplitSentencesInto and MemoFile User flagged two coverage gaps + asked whether SplitSentencesInto is even used. SplitSentencesInto IS used (paragraphstructure/rule.go:112 — the pool-friendly path that keeps the per-Check sentence []string off the alloc-budget gate). The function was at 100% line coverage via the rule's exercise but the package-level tests only hit it indirectly. Added direct tests: - TestSplitSentencesInto_EmptyReturnsDstUnchanged — covers the `strings.TrimSpace(text) == ""` early-return branch. - TestSplitSentencesInto_WhitespaceOnlyReturnsDstUnchanged — covers the equivalent whitespace-only path. - TestSplitSentencesInto_AppendsToProvidedSlice — pins the happy-path append semantics. - TestSplitSentencesInto_ReusesDstCapacity — pins the cap-reuse contract the rule's sync.Pool relies on. MemoFile (the *File-passing Memo variant introduced this PR) was at 0% coverage in internal/lint — it's only exercised through astutil.CollectSectionParagraphs from the integration suite, which doesn't roll into the lint package's profile. The warm-path `return e.val` early return wasn't hit by any direct test. Added: - TestFile_MemoFile — three calls under the same key, asserting build runs once and the *File argument matches the caller. - TestFile_MemoFile_ConcurrentSingleBuild — 50 goroutines on the same key, drives the mutex-guarded inner check. - TestFile_MemoFile_IndependentFromMemo — registering under Memo then reading via MemoFile (or vice versa) shares the same scratch entry; pins the composition contract. Coverage now: - internal/mdtext: 100% (SplitSentencesInto: 100%) - internal/lint: MemoFile 100% (was 0%), Memo 100% - internal/punkt: 100% - internal/rules/paragraphstructure: 97% (the un-100% functions are sentencePreview / ApplySettings, pre-existing and out of this PR's scope) Alloc gate unchanged at 9/9. * Memo / MemoFile panic safety + PR description CJK alignment Two real issues from Copilot review (round 11), each driven through red/green TDD: - lint/file.go Memo + MemoFile: build() ran while holding e.mu with no `defer e.mu.Unlock()`. A panic inside build would leave the mutex locked forever and deadlock every later Memo call on the same File. Added TestFile_Memo_PanicReleasesMutex and TestFile_MemoFile_PanicReleasesMutex — both deadlock against the un-deferred Unlock (verified locally) and pass against the new `defer e.mu.Unlock()` + `defer e.done.Store(true)` pair (which matches sync.Once's "panic still marks done" semantics so a panicking key doesn't keep re-running its build). - PR description vs internal/punkt/doc.go on CJK: the description still said "CJK punctuation support is dropped" even though the fork's IsCjkPunct + 。 sentence-break logic is intentionally retained. Updated the description to match doc.go: CJK terminal punctuation IS supported at upstream's English-pipeline level (。 as sent break, !? as word boundaries only). Alloc gate unchanged at 9/9 — the new defers are inlined by the Go compiler and add no per-call allocations. * Address Copilot review (round 12): correct doc claims and pin pool retention Three comment / rationale issues: - rule.go:123 — the message-construction comment said the build is "one alloc". Accurate only for ints 0–99 (where strconv.Itoa returns from the precomputed `smallsString` table without allocating). For larger ints Itoa allocates a string each call. Rewritten to describe the actual allocation profile: Sprintf is ~3, concat+Itoa is 1–3 depending on the int values, MDS024's typical inputs (max-sentences ~6, count ~7–30) hit the Itoa cache and land at 1 alloc. - mdtext.go:210 — the initTokenizerOnce comment claimed `sync.Once.Do(initTokenizer)` allocates a function value per call. Reviewer pointed out (correctly) that passing a package- level function to Do is allocation-free; only closures and method values force the boxing. Rewritten to admit OnceFunc is a stylistic choice (cleaner call site), not a perf win. - rule.go:160 + new test — reviewer claimed `clear(sentences)` before `sentences[:0]` was unnecessary overhead, citing GC-doesn't-scan-past-len. That's wrong: the GC scans the slice backing array across its allocated capacity. Verified empirically (cmd/poolprobe and the new test): a sync.Pool entry with `*[]string` retains the prior string at index 0 after a `[:0]+Put` round-trip; only `clear()` zeroes the slot. New TestSentBufPool_ClearReleasesStringReferences pins the contract with both halves (without-clear retains, with-clear doesn't) so a regression that drops `clear()` would fail the test. The in-code comment now references the empirical reasoning. No behaviour change; alloc gate still passes at 9/9. --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 1a45e45 commit a1aa6c5

53 files changed

Lines changed: 4328 additions & 989 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/workflows/release.yml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,16 @@ jobs:
330330
for pkg in npm/dist/*; do
331331
(cd "$pkg" && npm publish --access public --provenance)
332332
done
333+
- name: Stage LICENSE for root package
334+
# The root @mdsmith/cli package directory has no checked-in
335+
# LICENSE because the canonical one lives at repo root. npm
336+
# auto-includes a top-level LICENSE in the published tarball,
337+
# but only if it sits next to package.json at publish time —
338+
# so copy it in before `npm publish`. The root LICENSE also
339+
# carries the vendored neurosnap/sentences MIT notice for
340+
# internal/punkt/, which is what makes this step legally
341+
# required for the npm channel.
342+
run: cp LICENSE npm/mdsmith/LICENSE
333343
- name: Publish root package
334344
working-directory: npm/mdsmith
335345
run: npm publish --access public --provenance

LICENSE

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,3 +19,39 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
1919
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
2020
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
2121
SOFTWARE.
22+
23+
================================================================================
24+
THIRD-PARTY LICENSES
25+
================================================================================
26+
27+
This project incorporates source code from the following third-party projects.
28+
Each project's license appears verbatim below; a copy also lives alongside the
29+
vendored code in the indicated directory.
30+
31+
--------------------------------------------------------------------------------
32+
internal/punkt/ — derived from github.com/neurosnap/sentences v1.1.2
33+
(https://github.com/neurosnap/sentences/tree/v1.1.2)
34+
Verbatim copy: internal/punkt/UPSTREAM_LICENSE
35+
--------------------------------------------------------------------------------
36+
37+
The MIT License (MIT)
38+
39+
Copyright (c) 2015 Eric Bower
40+
41+
Permission is hereby granted, free of charge, to any person obtaining a copy
42+
of this software and associated documentation files (the "Software"), to deal
43+
in the Software without restriction, including without limitation the rights
44+
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
45+
copies of the Software, and to permit persons to whom the Software is
46+
furnished to do so, subject to the following conditions:
47+
48+
The above copyright notice and this permission notice shall be included in all
49+
copies or substantial portions of the Software.
50+
51+
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
52+
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
53+
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
54+
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
55+
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
56+
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
57+
SOFTWARE.

PLAN.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,6 @@ footer: |
119119
| 190 || opus | [Intra-file rule parallelism for non-NodeChecker rules](plan/190_intra-file-rule-parallelism.md) |
120120
| 191 || opus | [Hand-rolled DFA for Punkt's `reAbbr` to skip regex backtracking](plan/191_punkt-reabbr-dfa.md) |
121121
| 192 || opus | [Run-scoped read cache for catalog cross-host redundancy](plan/192_catalog-run-scoped-readcache.md) |
122-
| 193 | 🔲 | opus | [Rework MDS024 to fit the per-rule allocation budget (≤ 10 allocs/op)](plan/193_mds024-allocation-budget.md) |
122+
| 193 | | opus | [Rework MDS024 to fit the per-rule allocation budget (≤ 10 allocs/op)](plan/193_mds024-allocation-budget.md) |
123123
| 194 || opus | [Frontpage persona audit — reduce AI-first framing, surface non-AI path](plan/194_frontpage-persona-audit.md) |
124124
<?/catalog?>

internal/lint/file.go

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os"
77
"sort"
88
"sync"
9+
"sync/atomic"
910

1011
"github.com/yuin/goldmark/ast"
1112
"github.com/yuin/goldmark/parser"
@@ -108,10 +109,19 @@ type File struct {
108109

109110
// memoEntry guards a single Memo key so build runs exactly once even
110111
// when several rule passes (or concurrent LSP readers) race for the
111-
// same key.
112+
// same key. atomic.Bool + mutex is used instead of sync.Once because
113+
// once.Do takes a function value as a parameter — the closure
114+
// `func() { e.val = build() }` Memo would pass captures `e` and
115+
// `build`, both escape-tracking pointers, so it allocates per call.
116+
// On hot per-File memos (astutil.CollectSectionParagraphs feeds
117+
// every paragraph-aware rule), that single closure escape is the
118+
// dominant per-Check allocation the MDS024 budget gate sees. The
119+
// atomic flag is a double-checked-lock pattern: cheap atomic load
120+
// on the warm path, mutex-guarded build on the cold path.
112121
type memoEntry struct {
113-
once sync.Once
114122
val any
123+
done atomic.Bool
124+
mu sync.Mutex
115125
}
116126

117127
// Memo returns the value for key, computing it once via build on the
@@ -123,10 +133,53 @@ type memoEntry struct {
123133
// passes — three globs and front-matter reads of every matched file
124134
// per directive. The File is discarded after each Check, so nothing
125135
// is cached across files or runs.
136+
//
137+
// build is invoked directly (no wrapping closure) so the call adds
138+
// no per-Memo-call allocation beyond the cold-path memoEntry itself.
139+
//
140+
// Panic safety mirrors sync.Once: if build panics, the entry is
141+
// still marked done (via the deferred Store) and the mutex is
142+
// released (via the deferred Unlock), so the panic propagates
143+
// without leaving the per-File memo in a deadlocked state.
144+
// Subsequent calls on the same key serve the zero-value cached
145+
// result instead of re-running build, matching upstream sync.Once.
126146
func (f *File) Memo(key string, build func() any) any {
127147
ei, _ := f.scratch.LoadOrStore(key, &memoEntry{})
128148
e := ei.(*memoEntry)
129-
e.once.Do(func() { e.val = build() })
149+
if e.done.Load() {
150+
return e.val
151+
}
152+
e.mu.Lock()
153+
defer e.mu.Unlock()
154+
if !e.done.Load() {
155+
defer e.done.Store(true)
156+
e.val = build()
157+
}
158+
return e.val
159+
}
160+
161+
// MemoFile is the *File-passing variant of Memo: build receives this
162+
// File as an argument instead of capturing it in a closure. Callers
163+
// whose build needs nothing beyond File data can pass a package-
164+
// level function value, which avoids the per-call closure allocation
165+
// the plain `Memo` form forces on every invocation. The hot
166+
// astutil.CollectSectionParagraphs path is the canonical user.
167+
//
168+
// Panic safety matches Memo's contract: defer Unlock + defer
169+
// done.Store(true) keep the per-entry mutex from leaking a lock and
170+
// match sync.Once's "panic still marks done" semantics.
171+
func (f *File) MemoFile(key string, build func(*File) any) any {
172+
ei, _ := f.scratch.LoadOrStore(key, &memoEntry{})
173+
e := ei.(*memoEntry)
174+
if e.done.Load() {
175+
return e.val
176+
}
177+
e.mu.Lock()
178+
defer e.mu.Unlock()
179+
if !e.done.Load() {
180+
defer e.done.Store(true)
181+
e.val = build(f)
182+
}
130183
return e.val
131184
}
132185

internal/lint/file_test.go

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"sync"
66
"sync/atomic"
77
"testing"
8+
"time"
89

910
"github.com/stretchr/testify/assert"
1011
"github.com/stretchr/testify/require"
@@ -264,3 +265,153 @@ func TestFile_Memo_ConcurrentSingleBuild(t *testing.T) {
264265
assert.Equal(t, int32(1), atomic.LoadInt32(&calls),
265266
"build must run exactly once under concurrent access")
266267
}
268+
269+
// TestFile_MemoFile pins the same contract Memo has — build runs
270+
// exactly once per key, every later call returns the cached value
271+
// (the warm-path `return e.val` early-out), and distinct keys are
272+
// independent — for the *File-passing variant. MemoFile exists so
273+
// callers can register a package-level builder instead of a closure
274+
// that captures the File; the *File argument is what makes that
275+
// possible. The build also asserts it received the same *File the
276+
// caller passed.
277+
func TestFile_MemoFile(t *testing.T) {
278+
f := &File{Path: "t.md"}
279+
280+
var calls int32
281+
var receivedFile *File
282+
build := func(arg *File) any {
283+
atomic.AddInt32(&calls, 1)
284+
receivedFile = arg
285+
return 42
286+
}
287+
288+
require.Equal(t, 42, f.MemoFile("k", build),
289+
"first call must compute and return the value")
290+
require.Equal(t, 42, f.MemoFile("k", build),
291+
"second call must hit the e.done.Load() warm-path return")
292+
require.Equal(t, 42, f.MemoFile("k", build),
293+
"third call must also serve the cached value")
294+
assert.Equal(t, int32(1), atomic.LoadInt32(&calls),
295+
"build must run exactly once per key under MemoFile")
296+
assert.Same(t, f, receivedFile,
297+
"build must receive the same *File the caller passed")
298+
299+
var otherCalls int32
300+
require.Equal(t, "v2", f.MemoFile("k2", func(arg *File) any {
301+
atomic.AddInt32(&otherCalls, 1)
302+
return "v2"
303+
}))
304+
assert.Equal(t, int32(1), atomic.LoadInt32(&otherCalls))
305+
assert.Equal(t, int32(1), atomic.LoadInt32(&calls),
306+
"a distinct key must not re-run the first key's build")
307+
}
308+
309+
// TestFile_MemoFile_ConcurrentSingleBuild pins that build runs
310+
// exactly once even under concurrent readers — the mutex-guarded
311+
// inner check after the atomic.Bool fast-path is what enforces
312+
// "run-once" between two goroutines that both find done=false on
313+
// first read. Drives both paths of the double-checked-lock pattern.
314+
func TestFile_MemoFile_ConcurrentSingleBuild(t *testing.T) {
315+
f := &File{Path: "t.md"}
316+
317+
var calls int32
318+
var wg sync.WaitGroup
319+
for i := 0; i < 50; i++ {
320+
wg.Add(1)
321+
go func() {
322+
defer wg.Done()
323+
v := f.MemoFile("shared", func(arg *File) any {
324+
atomic.AddInt32(&calls, 1)
325+
return "once"
326+
})
327+
assert.Equal(t, "once", v)
328+
}()
329+
}
330+
wg.Wait()
331+
assert.Equal(t, int32(1), atomic.LoadInt32(&calls),
332+
"build must run exactly once under concurrent MemoFile access")
333+
}
334+
335+
// TestFile_MemoFile_IndependentFromMemo pins that the MemoFile and
336+
// Memo entry-points share the same scratch map — registering a key
337+
// under one is visible under the other. Without this, the per-key
338+
// dedup wouldn't compose when a future caller mixes the two forms.
339+
func TestFile_MemoFile_IndependentFromMemo(t *testing.T) {
340+
f := &File{Path: "t.md"}
341+
f.Memo("shared", func() any { return "via-Memo" })
342+
// MemoFile under the same key must hit the cached entry the
343+
// first call populated (the build below must not run).
344+
var memoFileCalls int32
345+
got := f.MemoFile("shared", func(_ *File) any {
346+
atomic.AddInt32(&memoFileCalls, 1)
347+
return "via-MemoFile"
348+
})
349+
assert.Equal(t, "via-Memo", got,
350+
"MemoFile must return the value the prior Memo call cached")
351+
assert.Equal(t, int32(0), atomic.LoadInt32(&memoFileCalls),
352+
"the MemoFile build must not run when Memo populated the key")
353+
}
354+
355+
// TestFile_Memo_PanicReleasesMutex pins that a panicking build does
356+
// not leave the per-entry mutex locked. Without `defer e.mu.Unlock()`
357+
// inside Memo, a panic inside the build would deadlock every later
358+
// Memo call on the same File (including under -race, where the
359+
// mutex order is checked). The recover here simulates a caller
360+
// that catches the panic; the subsequent Memo call must complete
361+
// within a short deadline rather than block forever.
362+
func TestFile_Memo_PanicReleasesMutex(t *testing.T) {
363+
f := &File{Path: "t.md"}
364+
365+
func() {
366+
defer func() { _ = recover() }()
367+
f.Memo("panicky", func() any {
368+
panic("boom")
369+
})
370+
}()
371+
372+
// If the mutex stayed locked, a second Memo call on the same
373+
// key would block forever. Run it in a goroutine with a hard
374+
// deadline so the test fails fast on a regression.
375+
done := make(chan any, 1)
376+
go func() {
377+
done <- f.Memo("panicky", func() any { return "recovered" })
378+
}()
379+
select {
380+
case got := <-done:
381+
// sync.Once-style semantics: the first build marked the
382+
// key done before propagating the panic, so subsequent
383+
// calls serve the zero-value cached result without re-
384+
// running the build. Either behaviour is acceptable as
385+
// long as the call returns; the test pins "no deadlock".
386+
t.Logf("Memo after panic returned %#v", got)
387+
case <-time.After(2 * time.Second):
388+
t.Fatal("Memo deadlocked after a panicking build — " +
389+
"the per-entry mutex was not released")
390+
}
391+
}
392+
393+
// TestFile_MemoFile_PanicReleasesMutex is the MemoFile counterpart
394+
// of TestFile_Memo_PanicReleasesMutex — the same defer-Unlock
395+
// guarantee must hold for the *File-passing variant.
396+
func TestFile_MemoFile_PanicReleasesMutex(t *testing.T) {
397+
f := &File{Path: "t.md"}
398+
399+
func() {
400+
defer func() { _ = recover() }()
401+
f.MemoFile("panicky", func(*File) any {
402+
panic("boom")
403+
})
404+
}()
405+
406+
done := make(chan any, 1)
407+
go func() {
408+
done <- f.MemoFile("panicky", func(*File) any { return "recovered" })
409+
}()
410+
select {
411+
case got := <-done:
412+
t.Logf("MemoFile after panic returned %#v", got)
413+
case <-time.After(2 * time.Second):
414+
t.Fatal("MemoFile deadlocked after a panicking build — " +
415+
"the per-entry mutex was not released")
416+
}
417+
}

internal/mdtext/abbr.go

Lines changed: 0 additions & 81 deletions
This file was deleted.

0 commit comments

Comments
 (0)