Close linter doc-sync gap by bridging registry, prose docs, and spec labels - #47204
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Pure test improvement — strengthens doc-sync invariants in
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47204 does not have the 'implementation' label and has 66 new lines of code in business logic directories (≤100 threshold). |
There was a problem hiding this comment.
Pull request overview
Adds test coverage linking the linter registry, prose documentation, and spec labels to prevent drift.
Changes:
- Adds registry-to-documentation set comparisons.
- Validates
documentedAnalyzers()labels against README entries. - Refactors documentation parsing into reusable helpers.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/doc_sync_test.go |
Adds cross-surface synchronization tests and parsing helpers. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/linters/doc_sync_test.go:117
- This scans every table in the README rather than only the
Subpackagestable. A lowercase backticked key in another table would be treated as an analyzer and could even mask a missing Subpackages row, so the new assertions do not enforce the stated invariant. Scope parsing from the### Subpackagesheading to the next heading.
for line := range strings.SplitSeq(string(readmeBytes), "\n") {
if !strings.HasPrefix(line, "| `") {
continue
}
m := readmeTableRe.FindStringSubmatch(line)
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
There was a problem hiding this comment.
Review: Close linter doc-sync gap
The approach is sound — bridging all three surfaces (registry, doc.go, README) in a single test is a real improvement over the previous disconnected checks.
One maintainability concern:
The docSlugToAnalyzerName switch is a fragile hardcoded mapping. The general rule (strip hyphens) already covers all existing cases, including panic-in-library-code → panicinlibrarycode. Replacing the switch with strings.ReplaceAll(slug, "-", "") would eliminate the need to remember to update it whenever a new hyphenated analyzer is added. See inline comment for the suggested fix.
Otherwise the refactor is clean — shared regexes, focused helper parsers, and clear error messages.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 23 AIC · ⌖ 4.65 AIC · ⊞ 5K
| return "panicinlibrarycode" | ||
| default: | ||
| return slug | ||
| } |
There was a problem hiding this comment.
The docSlugToAnalyzerName function uses a hardcoded switch with a single special case for "panic-in-library-code". The conversion rule (strip hyphens) is actually a general one: any hyphenated slug maps to an analyzer name by strings.ReplaceAll(slug, "-", ""). This hardcoded switch will silently need updating every time a new hyphenated analyzer is added.
Consider replacing with:
func docSlugToAnalyzerName(slug string) string {
return strings.ReplaceAll(slug, "-", "")
}This is simpler, future-proof, and consistent with how all existing analyzer names are formed.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking observations
Two medium-priority maintainability issues worth addressing:
-
** hardcoded switch** (line 108): The single-case mapping has no documented contract, no enforcement that new slugs are explicitly handled, and will produce confusing set-diff failures when a future hyphenated analyzer is added. At minimum, add a comment stating the general rule and consider validating outputs against the registry inside the helper.
-
Third assertion's coverage gap (line 51): The
readmeSlugs ↔ documentedLabelsassertion doesn't anchor to the registry, so a linter absent from both README anddocumentedAnalyzers()simultaneously passes silently. Each assertion should carry a brief comment explaining what it independently guarantees so future maintainers can reason about coverage.
No blocking bugs — the new invariants are a net improvement. Both issues are about future-proofing the test itself.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 25.4 AIC · ⌖ 4.75 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/linters/doc_sync_test.go:108
docSlugToAnalyzerName is a hardcoded manual mapping that will silently break for future analyzers. When a new hyphenated analyzer is added, the test will fail with a confusing set-mismatch rather than guiding the developer to add a mapping entry.
<details>
<summary>💡 Suggested fix</summary>
Document the invariant and optionally validate outputs against the registry at test time:
// docSlugToAnalyzerName maps a doc/README slug to its linters.All() Name.
// General rule: slugs equ…
</details>
<details><summary>pkg/linters/doc_sync_test.go:51</summary>
**Third assertion cross-checks README against `documentedAnalyzers()` but not against the registry — a linter omitted from both README and spec simultaneously still passes.** This creates a false confidence that all three surfaces are mutually consistent.
<details>
<summary>💡 Details</summary>
The chain is:
1. `registrySet` ↔ `docAsAnalyzerNames` ✓
2. `registrySet` ↔ `readmeAsAnalyzerNames` ✓
3. `readmeSlugs` ↔ `documentedLabels` ✓
If a linter is removed from `linters.All()` but its slug st…
</details>There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on two maintainability issues.
📋 Key Themes & Highlights
Key Themes
- Fragile slug→name mapping:
docSlugToAnalyzerNamehardcodes a single case instead of deriving the rule algorithmically (strings.ReplaceAll(slug, "-", "")). A second hyphenated analyzer will silently break the new test. - Unexplained filter: the
"internal"skip inparseReadmeSubpackageSetlacks a comment, making it opaque.
Positive Highlights
- ✅ The new three-way invariant (
linters.All()↔doc.go↔ README) is exactly the right guard — it closes the gap described in #47190. - ✅ Extracting
parseDocBulletSet/parseReadmeSubpackageSetas helpers eliminates duplicated parsing logic cleanly. - ✅ Hoisting regex vars to package level is the correct Go pattern.
- ✅ The
documentedAnalyzers().label↔ README assertion enforces the "derived from README" contract that was previously just a comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 32 AIC · ⌖ 4.7 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/linters/doc_sync_test.go:141
[/codebase-design] The hardcoded switch in docSlugToAnalyzerName will silently break when another hyphenated-package analyzer is added — the mapping rule is implicit rather than derived.
<details>
<summary>💡 Suggested fix</summary>
Replace the switch with the underlying rule:
func docSlugToAnalyzerName(slug string) string {
return strings.ReplaceAll(slug, "-", "")
}All existing analyzer Name values are the directory name with hyphens stripped (panic-in-library-code …
pkg/linters/doc_sync_test.go:121
[/codebase-design] The magic string "internal" filter has no comment explaining why the internal subpackage is excluded — this will confuse the next reader who sees it.
<details>
<summary>💡 Suggested fix</summary>
Add an inline comment:
if m[1] == "internal" {
continue // the internal helper package is not a public analyzer subpackage
}</details>
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (3 tests)
Per-Test Analysis:
Verdict
Key Strengths:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100 Excellent. 0% implementation tests (threshold: 30%). All 3 tests verify design contracts and user-visible specifications. New test TestDocSurfacesMatchRegistryAndSpecList directly addresses recurring doc-sync gap. Pure refactoring improves maintainability without changing production code.
|
🎉 This pull request is included in a new release. Release: |
Current doc-sync guards enforced two disconnected invariants:
linters.All()↔documentedAnalyzers()anddoc.go↔README.md. That allowed code/docs drift (and staledocumentedAnalyzers().labelvalues) to pass if each cluster stayed internally consistent.Bridge registry ↔ prose docs (new invariant)
TestDocSurfacesMatchRegistryAndSpecListinpkg/linters/doc_sync_test.go.doc.gobulletsREADME.mdSubpackages tablelinters.All().Validate
documentedAnalyzers()labels against READMEdocumentedAnalyzers().labelset exactly matches parsed README subpackage slugs.Refactor parser reuse in tests
docBulletRe,readmeTableRe).