Skip to content

feat(enrichment): doc-comment-vs-signature drift analyzer - #1794

Merged
JSONbored merged 2 commits into
JSONbored:mainfrom
nickmopen:feat/enrichment-doc-comment-drift
Jun 30, 2026
Merged

feat(enrichment): doc-comment-vs-signature drift analyzer#1794
JSONbored merged 2 commits into
JSONbored:mainfrom
nickmopen:feat/enrichment-doc-comment-drift

Conversation

@nickmopen

Copy link
Copy Markdown
Contributor

Summary

Adds a REES analyzer (#1519) that flags a verifiable doc-vs-code mismatch the PR introduced: a JSDoc/TSDoc @param tag naming a parameter the adjacent function no longer declares (the signature changed but the doc was left stale). The hunk often splits the doc block from the signature, so it fetches the full changed file at headSha (one authed contents fetch) and parses it. Additive + fail-safe — fills its own docCommentDrift findings key.

Deliberately conservative for precision (no fuzzy guessing):

  • Only named function declarations whose parameters are confidently enumerable — any destructuring ({ }/[ ]) or arrow-default param makes the set ambiguous, so the whole function is skipped.
  • Only functions whose signature the PR actually changed (a signature line is in the patch's added set) — pre-existing drift elsewhere is not reported.
  • Only top-level @param names; nested @param opts.x tags reference an existing param and are ignored.
  • Non-source and test files are skipped; a missing token/headSha or any fetch error fails safe (no finding).

Implementation (established review-enrichment/ pattern)

  1. DocCommentDriftFinding type + docCommentDrift? key in src/types.ts
  2. src/analyzers/doc-comment-drift.ts — pure helpers (addedLineNumbers, parseDocParams, parseFunctionParams, findDocCommentDrift) + scanDocCommentDrift(req, fetch, opts); reuses the codeowners-style contents fetch
  3. Registered in the src/brief.ts ANALYZERS registry
  4. Public-safe block in src/render.ts (file:line + symbol + stale param names only)
  5. node:test units in a separate test/doc-comment-drift.test.ts

Validation

From review-enrichment/ (Node 24):

npm test   # build + node --test: 235 pass / 0 fail (16 new doc-comment-drift units)

Covers added-line parsing, doc/param extraction (typed/optional/nested/rest/this/generics-with-comma), destructuring→skip, multi-line signatures, untouched-function exclusion, fail-safe fetch (missing token/headSha, non-ok, throw, abort), non-source skip, and the rendered block.

Closes #1519

@nickmopen
nickmopen requested a review from JSONbored as a code owner June 29, 2026 23:55
@dosubot dosubot Bot added the size:L label Jun 29, 2026
@loopover-orb

loopover-orb Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-06-30 06:45:31 UTC

6 files · 1 AI reviewer · no blockers · readiness 55/100 · CI green · unknown

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The PR adds a conservative doc-comment drift analyzer, wires it into the analyzer registry, renders its findings, and covers the main pure helpers plus scan/render behavior. The visible implementation is fail-closed on fetch/reconstruction/parsing problems and avoids the earlier advisory concerns by reconstructing old signatures instead of keying off removed identifiers. I do not see a reachable correctness defect in the provided changed files, but the parser has intentional recall limits that should be made more explicit in tests/docs if this analyzer is expected to catch common exported function forms.

Nits — 6 non-blocking
  • nit: review-enrichment/src/analyzers/doc-comment-drift.ts:19 only matches declarations whose `function name(` prefix is on one line, so common formatted declarations like `export function f\n(` are silently skipped; add a test documenting that deliberate recall limit or broaden the matcher.
  • nit: review-enrichment/src/analyzers/doc-comment-drift.ts:244 reports the function declaration line, not the stale `@​param` line, which is actionable enough but less precise than other analyzer findings in this service.
  • nit: review-enrichment/src/analyzers/doc-comment-drift.ts:139 intentionally fails closed on parameter types with generic commas, so `Map<K, V>` signatures are skipped even though they are common TypeScript; consider documenting that limitation in the descriptor notes as well as the code comment.
  • Add a focused test for a formatted declaration where `function f` and `(` are on separate lines, either asserting the skip or locking in support if you broaden `FUNC_DECL_RE`.
  • Consider returning stale doc tag line numbers from `parseDocParams` so `renderBrief` can point reviewers at the exact stale `@​param` line instead of only the function line.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #1519
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:L; 1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR; address the blocker before review.
Contributor workload ✅ 10/10 Author activity: 89 registered-repo PR(s), 60 merged, 1 issue(s).
Contributor context ✅ Confirmed Gittensor contributor nickmopen; Gittensor profile; 89 PR(s), 1 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
Contributor next steps
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Fix the blocker.
  • Triage stale or unlinked PRs.
  • Refresh registry data or choose a registered active repo.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. labels Jun 29, 2026
@nickmopen
nickmopen force-pushed the feat/enrichment-doc-comment-drift branch from a019120 to 21bacd8 Compare June 30, 2026 00:24
@nickmopen
nickmopen force-pushed the feat/enrichment-doc-comment-drift branch 3 times, most recently from abf9d13 to a9ab1e9 Compare June 30, 2026 01:09
@nickmopen
nickmopen force-pushed the feat/enrichment-doc-comment-drift branch 8 times, most recently from 632c2d0 to 088ad59 Compare June 30, 2026 05:48
Add a REES analyzer that flags a verifiable doc-vs-code mismatch the PR introduced: a JSDoc/TSDoc
`@param` tag that names a parameter the adjacent function no longer declares (a signature changed but
the doc was left stale). The hunk often splits the doc block from the signature, so it fetches the full
changed file at headSha (one authed contents fetch) and parses it.

Deliberately conservative for precision: only NAMED `function` declarations whose parameters are
confidently enumerable (any destructuring or arrow-default param → the function is skipped), only
functions whose signature the PR actually changed (added lines), and only top-level `@param` names
(nested `opts.x` tags reference an existing param and are ignored). Non-source and test files are skipped;
missing token/headSha and any fetch error fail safe.

Registered in the brief orchestrator and rendered as a public-safe block (file:line + symbol + the stale
param names). Tests in their own file.

Closes JSONbored#1519
@nickmopen
nickmopen force-pushed the feat/enrichment-doc-comment-drift branch from 088ad59 to d971655 Compare June 30, 2026 05:56

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The most important remaining risk is a real false negative around optional JSDoc parameter syntax with defaults, where stale docs can be silently missed.

Please create a followup PR to resolve this as well. Approving.

@dosubot dosubot Bot added the lgtm label Jun 30, 2026
@JSONbored
JSONbored merged commit d8b3e5b into JSONbored:main Jun 30, 2026
12 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 30, 2026
nickmopen added a commit to nickmopen/gittensory that referenced this pull request Jun 30, 2026
…n doc-comment drift

Follow-up to JSONbored#1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the JSONbored#1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.
nickmopen added a commit to nickmopen/gittensory that referenced this pull request Jun 30, 2026
…n doc-comment drift

Follow-up to JSONbored#1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the JSONbored#1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.
nickmopen added a commit to nickmopen/gittensory that referenced this pull request Jun 30, 2026
…n doc-comment drift

Follow-up to JSONbored#1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the JSONbored#1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.
nickmopen added a commit to nickmopen/gittensory that referenced this pull request Jun 30, 2026
…n doc-comment drift

Follow-up to JSONbored#1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the JSONbored#1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.
nickmopen added a commit to nickmopen/gittensory that referenced this pull request Jun 30, 2026
…n doc-comment drift

Follow-up to JSONbored#1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the JSONbored#1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.
nickmopen added a commit to nickmopen/gittensory that referenced this pull request Jun 30, 2026
…n doc-comment drift

Follow-up to JSONbored#1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the JSONbored#1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.
nickmopen added a commit to nickmopen/gittensory that referenced this pull request Jun 30, 2026
…n doc-comment drift

Follow-up to JSONbored#1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the JSONbored#1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.
nickmopen added a commit to nickmopen/gittensory that referenced this pull request Jun 30, 2026
…n doc-comment drift

Follow-up to JSONbored#1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the JSONbored#1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.
JSONbored pushed a commit that referenced this pull request Jun 30, 2026
…n doc-comment drift (#1875)

Follow-up to #1794. `parseFunctionParams` split parameter lists on every top-level comma without tracking
generic `<…>`, so a comma inside a generic type or default value (e.g. `cache = new Map<string, number>()`
or `a: Map<K, V>`) split mid-generic, produced an unparseable fragment, and made the whole function skip —
silently MISSING a documented-but-removed parameter (the false negative noted on the #1794 review).

Make `splitParams` generic-aware: a `<` with a balanced matching `>` is treated as one opaque generic
argument list (its commas don't split), while a `<`/`>` with no match stays a comparison operator. This
fixes the false negative, keeps comparison/arrow defaults enumerable, and removes the prior need to drop
type-argument fragments. Adds unit + end-to-end regressions.

Co-authored-by: Nick M <274344962+nickmopen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

feat(enrichment): Doc-comment-vs-signature drift

2 participants