feat(review): deterministic fallback categorization for findings (#2148)

[jimcody1995]

Summary

Implements the deterministic keyword/path fallback categorization from #2148.

• Adds the issue's named pure function inferFindingCategory(body: string, path: string): FindingCategory in src/review/finding-category-classify.ts, using the src/signals/path-matchers matchers (isTestPath, isDocsFile, isConfigFile) plus the existing keyword sets.
• Documented precedence (in JSDoc):
  1. Path signals first — test file → tests, docs file → style, config file → maintainability (the file the reviewer anchored to is the strongest deterministic signal).
  2. Keyword buckets over the body, costliest-miss-first — security → performance → tests → style → maintainability.
  3. Final default correctness when nothing matches.
• Makes classifyFindingCategory({ path, body }) a thin adapter that delegates to inferFindingCategory, so it stays single-sourced and every existing fallback site (inline-comment rendering, category tallies/collapsible) now gains docs/config path routing it previously lacked.
• Drops the heavy signals/local-branch import in favor of the path-only matchers (also lighter for ui:typecheck).

Note on "wire in the parser"

The deliverable mentions wiring the fallback "in the parser." The category pipeline landed in #3634 (#1958) after this issue was filed, and it deliberately leaves category absent on the parsed finding (parseModelReview) so callers that never asked for categories see no field — a contract locked in by ai-review.test.ts. The fallback is therefore applied at the resolution boundary (finding.category ?? classifyFindingCategory(...)), i.e. only when the model omits/garbles the category. This PR upgrades that single fallback engine rather than regressing #3634's tested contract; behavior for a present/valid model category is unchanged.

Test plan

• npx vitest run test/unit/finding-category-classify.test.ts — adds full per-branch coverage: every path signal, every keyword bucket, no-signal default, and precedence edges (path beats keywords; security beats performance).
• npx vitest run test/unit/finding-category-collapsible.test.ts test/unit/finding-category-tally.test.ts test/unit/inline-comments-select.test.ts test/unit/inline-comments.test.ts test/unit/ai-review.test.ts — all green, no behavior change.
• npm run typecheck
• Module patch coverage: 100% statements / branches / functions / lines.

Closes #2148.

Made with Cursor

Made with Cursor

[superagent-security]

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[loopover-orb]

Tip

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

✅ Gittensory review result - approve/merge recommended

_{Review updated: 2026-07-07 07:17:13 UTC}

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

✅ Suggested Action - Approve/Merge

• safe to merge

Review summary
This PR extracts the existing classifyFindingCategory logic into a pure inferFindingCategory(body, path) function and adds two new path-based routing rules (docs → style, config → maintainability) ahead of the keyword buckets, with classifyFindingCategory becoming a thin adapter. The precedence is clearly documented and thoroughly tested (every path signal, every keyword bucket, and cross-precedence cases), and CI is green. The one behavioral risk is that the old code checked `isTestPath(path) || isTestFile(path)` for the tests bucket, and this diff drops the `isTestFile` (from `signals/local-branch`) check entirely, relying only on `isTestPath` — if `isTestFile` matched any test-path shapes that `isTestPath` doesn't, some previously-`tests`-categorized findings could now fall through to keyword matching or `correctness`.

Nits — 5 non-blocking

• src/review/finding-category-classify.ts:29 drops the `isTestFile` (from `signals/local-branch`) half of the old `isTestPath(path) || isTestFile(path)` test-path check — confirm `isTestFile`'s matching set is a strict subset of `isTestPath`'s before relying on `isTestPath` alone, since a divergence would silently change categorization for some test-file findings.
• The external brief's 'magic numbers 2148/3' flags are false positives (issue number in a comment, count of list items in a docstring) — not real numeric literals needing a named constant.
• docs/architecture.md and README.md tests only exercise the 'docs → style' path with generic bodies; consider one negative case where a docs-pathed finding has strong test/maintainability wording to double-confirm doc-path precedence over those buckets too, not just security.
• Add a short note in the PR description (or a code comment) confirming `isTestFile` was verified redundant with `isTestPath` before removal, since that's the only semantic narrowing in this diff.
• Consider a test asserting `classifyFindingCategory`'s object-shaped signature is unchanged for existing callers (a compile-time/type-level check), since this is the contract most likely to be depended on outside this file.

Signal	Result	Evidence
Code review	✅ No blockers	1 reviewer
Linked issue	✅ Linked	#2148
Related work	✅ No active overlap found	No same-issue or scoped active PR overlap found.
Change scope	✅ 20/20	Low review scope from cached public metadata (1 linked issue).
Validation posture	✅ 25/25	PR body includes validation/test evidence.
Contributor workload	✅ 10/10	Author activity: 149 registered-repo PR(s), 95 merged, 0 issue(s).
Contributor context	✅ Confirmed Gittensor contributor	jimcody1995; Gittensor profile; 149 PR(s), 0 issue(s).
Gate result	✅ Passing	No configured blocker found.

Review context

• Author: jimcody1995
• Role context: outside_contributor
• Public audience mode: oss maintainer
• Lane context: Repository is configured for direct PR review.
• Public profile languages: TypeScript, JavaScript
• Official Gittensor activity: 149 PR(s), 0 issue(s).
• PR-specific overlap: none found.

Contributor next steps

• Keep the PR focused and include validation evidence before maintainer review.

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.66%. Comparing base (4942547) to head (6c47f95).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3934   +/-   ##
=======================================
  Coverage   93.66%   93.66%           
=======================================
  Files         372      372           
  Lines       34856    34859    +3     
  Branches    12743    12745    +2     
=======================================
+ Hits        32649    32652    +3     
  Misses       1588     1588           
  Partials      619      619           

Files with missing lines	Coverage Δ
src/review/finding-category-classify.ts	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[loopover-orb]

Gittensory approves — the gate is satisfied and CI is green.